Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dependent archive never run #18403

Merged
merged 7 commits into from Jan 7, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Nov 29, 2021

Description:

Fixes: #18362
Fix for dependent archive never run, not sure that's the best approach

Review

Peter Zhang added 2 commits November 30, 2021 12:08
set plugin not include UserCountry
update processer
@peterhashair peterhashair self-assigned this Nov 30, 2021
@peterhashair peterhashair marked this pull request as ready for review November 30, 2021 22:05
@peterhashair peterhashair added the Needs Review PRs that need a code review label Nov 30, 2021
@peterhashair peterhashair added this to the 4.7.0 milestone Nov 30, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 8, 2021
@sgiehl
Copy link
Member

sgiehl commented Dec 8, 2021

@peterhashair were you able to reproduce the reported issue? I don't think that specific solution is something we want to go with. We should check for a global solution, as if that can happen for one report, it can also happen for any similar report.

@peterhashair
Copy link
Contributor Author

@sgiehl I kind of do not fully understand that one, just quickly applied @tsteur solution, I will look deep for that one. remove the need review tag for now.

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Dec 9, 2021
@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Dec 9, 2021
@tsteur
Copy link
Member

tsteur commented Dec 9, 2021

@peterhashair we could catch up in a call tomorrow morning or afternoon if that helps? generally I would have thought that https://github.com/matomo-org/matomo/compare/4.x-dev...processdependentarchive?quick_pull=1#diff-0423f9c2b4343926ff8deebfed3605b3f1905daa9e8fecc1ec1f00f65bf3e997R683 should fix it.

remove hardcode plugin name
@sgiehl
Copy link
Member

sgiehl commented Dec 10, 2021

The changes are logically looking fine. I was wondering if we are able to reproduce an issue that those changes will fix. If so we could possibly add a test for that, to ensure that won't regress in the future.

@tsteur
Copy link
Member

tsteur commented Dec 12, 2021

fyi @sgiehl we actually noticed that there might not have been an actual issue and it worked potentially "by luck" as MediaAnalytics requested a different plugin to be archived and then it wouldn't ever return in that case: https://github.com/matomo-org/matomo/blob/4.6.2/core/ArchiveProcessor.php#L683 because plugin is different. Also in the case of "goals" it still worked probably by luck because the UI would also requested these metrics.

This kind of also means there might be another bug in there as even if the segment is already now being archived, in some cases it would archive it again (I think there's an issue for cron archiving but this check may be needed for browser archiving or if only a specific plugin is requested, quite difficult). I might create a separate issue for this.

A quick test could be maybe added though

@sgiehl
Copy link
Member

sgiehl commented Dec 20, 2021

@peterhashair could you check if it's possible to add a useful test for that?

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jan 5, 2022
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Jan 5, 2022
@peterhashair peterhashair merged commit 88797d7 into 4.x-dev Jan 7, 2022
@peterhashair peterhashair deleted the m-18362-dependent-archives-never-processed branch January 7, 2022 12:20
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependent archives never processed for All visits segment causing some data to not show
4 participants