Allow invalidated archive data to be used until they are re-processed by mkurzeja · Pull Request #6116 · matomo-org/matomo · GitHub
Skip to content

Allow invalidated archive data to be used until they are re-processed #6116

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Sep 21, 2014
Merged

Allow invalidated archive data to be used until they are re-processed #6116

merged 13 commits into from
Sep 21, 2014

Conversation

mkurzeja
Copy link
Contributor

@mkurzeja mkurzeja commented Sep 3, 2014

Added DONE_INVALIDATED flag to archiving and considering it when fetching archives

@mkurzeja
Copy link
Contributor Author

mkurzeja commented Sep 3, 2014

ref #5932

@mkurzeja
Copy link
Contributor Author

mkurzeja commented Sep 3, 2014

This is a small part of the code that shows how I plan to achieve the goal:

ArchiveSelector loads the invalidated data only if archiver will not be triggered in the current request. Thanks to that if archiver can be run it re-processes the data and if not it loads the invalidated ones.

Left to be done:

  1. Mark archive as invalid instead of deleting when CoreAdminHome.invalidateArchivedReports is run
  2. Force core:archive method to detect if site has invalid archives (right now --force-idsites and --force-date-range need to be added).
  3. Add invalid archive deletion in core:archive command

@mkurzeja
Copy link
Contributor Author

mkurzeja commented Sep 8, 2014

@mattab please check this PR. It should be ready to merge

mattab
mattab reviewed Sep 9, 2014
View reviewed changes
core/DataAccess/ArchivePurger.php
foreach ($archiveTables as $archiveTable) {
$query = '
SELECT t1.idarchive FROM `' . $archiveTable . '` t1
INNER JOIN `piwik_archive_numeric_2014_09` t2
Copy link
Member

Choose a reason for hiding this comment

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

table month is hardcoded here: piwik_archive_numeric_2014_09

@mattab
Copy link
Member

mattab commented Sep 9, 2014

@mkurzeja Nice work, I put feedback inline, looking forward to being able to merge it!

@mattab mattab added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels Sep 9, 2014
@mattab mattab added this to the Piwik 2.7.0 milestone Sep 9, 2014
@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Sep 9, 2014
@mkurzeja
Copy link
Contributor Author

@mattab Thanks for the review. I've introduced some changes and updated to the current master.

mattab
mattab reviewed Sep 17, 2014
View reviewed changes
core/DataAccess/ArchivePurger.php
INNER JOIN `' . $archiveTable . '` t2
ON t1.name = t2.name AND t1.idsite=t2.idsite
AND t1.date1=t2.date1 AND t1.date2=t2.date2 AND t1.period=t2.period
WHERE t1.value = 4 AND t2.value IN(' . ArchiveWriter::DONE_OK . ', ' . ArchiveWriter::DONE_OK_TEMPORARY . ') AND t1.name LIKE \'done%\'';
Copy link
Member

Choose a reason for hiding this comment

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

magic number 4 should be using the const

mattab
mattab reviewed Sep 17, 2014
View reviewed changes
tests/PHPUnit/Integration/ArchiveInvalidationTest.php
public function testAnotherApi($api, $params)
{
$this->setBrowserArchivingTriggering(1);
$this->invalidateTestArchives();
Copy link
Member

Choose a reason for hiding this comment

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

maybe this line can be removed as it's already called before in testSameApi

@mattab
Copy link
Member

mattab commented Sep 17, 2014

Nice. Besides review above, the tests are easy to read and understand!

Well done, looking forward to merge it.

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 17, 2014
@mkurzeja
Copy link
Contributor Author

@mattab Thanks for the review. I've added all changes and updated to the current master

mattab pushed a commit that referenced this pull request Sep 21, 2014
Merge pull request #6116 from mkurzeja/feature/5932-invalidate-archiv…
0fac10d 
…ed-reports

fixes #5932 Allow invalidated archive data to be used until they are re-processed
@mattab mattab merged commit 0fac10d into matomo-org:master Sep 21, 2014
@mattab
Copy link
Member

mattab commented Sep 21, 2014

Kuddos @mkurzeja for this nicely done Pull request and big usability improvement to Piwik users who will now enjoy reports filled with data until the reports will be re-processed. 👍

mattab added a commit that referenced this pull request Sep 21, 2014
@mattab
fixes #5933 New $period parameter to the API call CoreAdminHome.inval…
c41a805 
…idateArchivedReports refs #6116
mattab added a commit that referenced this pull request Sep 29, 2014
@mattab
Removing disablePurge hack in tests, as this should not be needed any…
bc782f4 
…more

refs #6116
@mattab mattab mentioned this pull request Sep 30, 2014
Merged
@mattab mattab mentioned this pull request Oct 23, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@mkurzeja @mattab