Don't browser archive when segment is set to be pre-processed #18123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The tests don't seem to fail because of this change. @mattab can you maybe also have a look at the logic and if that makes sense to you? |
tsteur
commented
Oct 11, 2021
|
|
||
| // Always allow processing one report | ||
| if (!$isArchivingEnabled | ||
| && (!self::isBrowserArchivingAvailableForSegments() || self::isSegmentPreProcessed($idSites, $segment)) |
There was a problem hiding this comment.
fyi looking at other usages of isBrowserArchivingAvailableForSegments this change should be fine as in relevant places we already check for browser archiving available || segment pre processed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
While commenting on #17941 I noticed that a segment that is selected to be pre-processed would still be archived when it's viewed from browser even though it's set to be preprocessed. When a segment is selected to be pre-processed then it should be only processed during cron archiving no matter the if browser archiving for segments is available or not.
I'm meaning this setting:
When it's currently set to be pre-processed, it would be still archived during day time. Users confirm this makes Matomo slow!
The logic for whether archiving is enabled / disabled or not is currently where complicated IMO:
I'm thinking this new logic is more clear and less prone to side effects from when the
shouldProcessReportsAllPluginsis shown.Behaviour to be specified
There's also one other issue I noticed in the implementation. When setting
archiving_range_force_on_browser_request=0in config, then we were still archiving ranges in some cases. Like if browser archiving is enabled (as described in the config setting for it) or if therangeis requested during CLI archiving. Not sure if that's on purpuse but I kept this logic.Review