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

Ensure all plugins are archived when range archiving is triggered by core:archive #14890

Merged
merged 2 commits into from Sep 20, 2019

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Sep 11, 2019

fixes #14889

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 11, 2019
@sgiehl sgiehl added this to the 3.12.0 milestone Sep 11, 2019
@@ -57,7 +57,7 @@ public static function getDoneStringFlagFor(array $idSites, $segment, $periodLab

public static function shouldProcessReportsAllPlugins(array $idSites, Segment $segment, $periodLabel)
{
if ($segment->isEmpty() && $periodLabel != 'range') {
if ($segment->isEmpty() && ($periodLabel != 'range' || SettingsServer::isArchivePhpTriggered())) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the range check makes sense at all. But at least all reports should be archived when triggered by core:archive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the check is just to make browser triggered archiving of ranges faster. Otherwise, I guess it might take a bit longer for the user to see reports. Not sure how much time it would actually add though...

@viezel
Copy link

viezel commented Sep 13, 2019

can anyone please review this PR? 👍
Its causing quite a lot of performance bottlenecks since these periods are not archived.

@diosmosis
Copy link
Member

Code looks good, can we add a test for this in ArchiveCronTest?

@sgiehl
Copy link
Member Author

sgiehl commented Sep 18, 2019

@diosmosis added a test

@diosmosis diosmosis merged commit b00220e into 3.x-dev Sep 20, 2019
@diosmosis diosmosis deleted the rangearchiving branch September 20, 2019 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review 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.

Range archives might not be fully processed by core:archive
3 participants