Remove temporary archive concept and just invalidate on tracking. #14639
Conversation
| $selector = new SegmentSelectorControl(); | ||
| $out .= $selector->render(); | ||
| } catch (\Exception $ex) { | ||
| print $ex->getTraceAsString()."\n"; |
There was a problem hiding this comment.
guess would be better to log the error instead of printing it...
There was a problem hiding this comment.
sorry, that's was for debugging something else, accidentally committed it
There was a problem hiding this comment.
btw this still needs to be removed right?
There was a problem hiding this comment.
@diosmosis this still needs to be removed, be good to not forget.
|
NOTE: this will require a change to CustomReports. Actually I'll edit to maintain BC. |
|
@diosmosis sounds good to change custom reports as long as we can still support previous Matomo versions as well. |
|
@tsteur changes should be BC now. However, I noticed AbTesting does some stuff w/ ArchiveWriter that includes a temporary archive (in GetMetricsOverview.php). Will this be a problem? Not sure what the code does. |
| { | ||
| $bindSQL = array($idSite, | ||
| $dateStartIso, | ||
| $dateEndIso, | ||
| $period, | ||
| ); | ||
|
|
||
| $timeStampWhere = ''; |
There was a problem hiding this comment.
@diosmosis do we maybe need an update script to get rid of all temporary archives? Maybe some temporary archives would need to be converted to a completed archive? (not sure if that's an issue but wondering if it's possible that we were sometimes reading a temporary archive because for some reason no completed archive was created)
There was a problem hiding this comment.
This code should still look for temporary archives in addition to done archives (whichever is latest), so it shouldn't be necessary to do it now. I could add the update though.
|
Looks promising so far @diosmosis . Should we maybe merge this with PR #14091 as we maybe still need the changes in |
@tsteur I'm not sure that PR as it is done is even needed anymore... If we only run archiving for invalidated/missing archives, then we don't even have to check for visits in log_visit. So instead of every other check, just see if there is an idarchive w/ DONE_OK for the site/period/segment/etc., and if not send the request. Otherwise, skip it. What do you think? Since we invalidate on tracking, I think this should work, ie, if there are no visits, the archive won't be invalidated. |
|
I meant eg in |
|
@tsteur Yes that's what I meant, but the previous PR checks if there are visits in log_visit. Here I'm suggesting just to check if there's an existing archive w/ |
|
Sounds good to do this in another plan, I'll review the PR again later this week then. 👍 Looks good so far though had a look already a few times. |
core/DataAccess/ArchiveSelector.php
Outdated
| @@ -45,7 +45,7 @@ private static function getModel() | |||
| return new Model(); | |||
| } | |||
|
|
|||
| public static function getArchiveIdAndVisits(ArchiveProcessor\Parameters $params, $minDatetimeArchiveProcessedUTC) | |||
| public static function getArchiveIdAndVisits(ArchiveProcessor\Parameters $params) | |||
There was a problem hiding this comment.
@diosmosis some usages still seem to pass two parameters here, eg Advertising, CustomReports, SearchEngineKeywordsPerformance, ... might be better to still keep the 2nd parameter but simply not use it. Otherwise it could break things and lead to notices etc. In Matomo 4 we got remove the 2nd parameter
| $selector = new SegmentSelectorControl(); | ||
| $out .= $selector->render(); | ||
| } catch (\Exception $ex) { | ||
| print $ex->getTraceAsString()."\n"; |
There was a problem hiding this comment.
@diosmosis this still needs to be removed, be good to not forget.
|
@diosmosis can you look at the 2 failing tests https://travis-ci.org/matomo-org/matomo/jobs/557621873#L829-L844 ? otherwise looks good to merge and work on the CronArchiver in another PR. Also left a comment where we possibly break the API and need to put back a parameter. One print debug is also there, otherwise feel free to merge once these things are done . |
CC @tsteur
Seems like it works fine, tested manually. Automated tests shouldn't pass just yet, will work on that soon.
Refs #14091