Refactor archivers to support multiple idsites #13168
Merged
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
tsteur
added
Pull Request WIP
tsteur
added
Needs Review
Any possibility to easily add some tests with multiple site ids now? |
tsteur
commented
Jul 16, 2018
| * In Matomo 4.0 we should maybe remove this event, and instead maybe always archive from raw data when it is daily archive, | ||
| * no matter if single site or not. We cannot do this in Matomo 3.X as some custom plugin archivers may not be able to handle multiple sites. | ||
| */ | ||
| Piwik::postEvent('ArchiveProcessor.shouldAggregateFromRawData', array(&$shouldAggregateFromRawData, $this->params)); |
There was a problem hiding this comment.
I've set the event to be ignored for now. I can see it may be useful if someone actually wanted to aggregate week or month from raw data as well but usually that doesn't happen. It will be useful though in the meantime not only for the tests but also for Roll-Up reporting which soon may or may not force the archiving from raw data when there are multiple sites.
|
@sgiehl added a test 👍 |
mattab
approved these changes
Jul 17, 2018
sgiehl
approved these changes
Jul 17, 2018
InfinityVoid
pushed a commit
to InfinityVoid/matomo
that referenced
this pull request
Oct 11, 2018
* refactor archivers to support multiple idsites * added a test to ensure multiple sites work * add comment
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.
refs #13162
After looking into the code this seems to be way easier to implement than I thought it would be. This is because the bind was already always supporting multiple sites but only the query was not. The reason this worked in the past was simply that it was never called with multiple idsites, otherwise it would have directly failed.