Fixes for specific case when partial archives have to initiate archiving for child archives #17439

diosmosis · 2021-04-09T05:33:23Z

Description:

Includes new test case and changes to make it pass. See inline comments for individual changes/fixes.

Review

…hecks for partial archives, fix a couple issues that arise when archiving a multi-period partial archive that has to initiate archiving for a child archive and reuse archives for a child archive.

diosmosis · 2021-04-09T05:35:56Z

core/ArchiveProcessor/Loader.php

@@ -105,6 +105,7 @@ public function prepareArchive($pluginName)
    private function prepareArchiveImpl($pluginName)
    {
        $this->params->setRequestedPlugin($pluginName);
+        $this->params->setIsPartialArchive(false);


Normally when configuring Params objects, we create new ones where partial archive is set to false to begin with. Here in Loader, however, we reuse Params objects to archive VisitsSummary by itself, and then the rest of the plugins. When reusing, we have to specifically reset this property, or other archives will think they are partial archives.

This is the only place this should be done, so I did not consider changing Params a good idea, since developers should only use the setIsPartialArchive method in Archiver instances for a specific use case.

@diosmosis in line 113 should it set $this->params->setArchiveOnlyReport($requestedReport) or maybe setArchiveOnlyReport() should set setIsPartialArchive automatically?

do we ever need to reset the original/previously set IsPartialArchive value maybe? haven't checked yet where it's reused

diosmosis · 2021-04-09T05:41:10Z

core/ArchiveProcessor/Loader.php

@@ -117,8 +118,7 @@ private function prepareArchiveImpl($pluginName)
        // with a ts_archived >= the DONE_OK/DONE_INVALIDATED date.
        list($idArchives, $visits, $visitsConverted, $isAnyArchiveExists) = $this->loadExistingArchiveIdFromDb();
        if (!empty($idArchives)
-            && !$this->params->getArchiveOnlyReport()


Removed this since it seemed safer to reuse existing archives if we're not forcing single plugin archiving, and not depend on this state which comes from a query param.

diosmosis · 2021-04-09T05:42:44Z

core/ArchiveProcessor/Loader.php

+                $metrics = $pluginsArchiver->callAggregateCoreMetrics();
+                $pluginsArchiver->finalizeArchive();
+                return $metrics;
+            });


Unset requestedReport so it's not picked up if we recurse here. If we're getting core metrics for a week, and there isn't a day archive already existing, we'll try to create the archive. But w/ requestedReport set, we may try to archive the other plugin as well as VisitsSummary. (Note: this can be seen in the new test if the code is removed.)

diosmosis · 2021-04-09T05:43:14Z

core/ArchiveProcessor/Rules.php

@@ -328,17 +328,20 @@ public static function getSelectableDoneFlagValues($includeInvalidated = true, P
        return $possibleValues;
    }

-    public static function isForceArchivingSinglePlugin()
+    public static function isRequestingToAndAbleToForceArchiveSinglePlugin()


Renamed to show it's true function more clearly.

diosmosis · 2021-04-09T05:48:39Z

core/ArchiveProcessor/Rules.php

+    public static function isActuallyForceArchivingSinglePlugin()
+    {
+        return Loader::getArchivingDepth() <= 1 && self::isRequestingToAndAbleToForceArchiveSinglePlugin();
+    }


We now have two methods to determine if we are forcing archiving of a single plugin:

isRequestingToAndAbleToForceArchiveSinglePlugin() which returns true if the original request was for a pluginOnly archive, and if it came from archive.php (so we're able to in that case).

and isActuallyForceArchivingSinglePlugin() which returns true if in this specific archiving request, we want to archive a pluginOnly archive. We only do this for the root request, for others we try to reuse existing archives. If no existing archives exist, archiving will launch for them so we don't need to do anything (unless, like in the Cloud plugin, we force disable it).

diosmosis · 2021-04-09T05:49:47Z

core/DataAccess/ArchiveSelector.php

+                        // sanity check: DONE_PARTIAL shouldn't be used w/ done archives, but in case we see one,
+                        // don't treat it like an all plugins archive
+                        && $value != ArchiveWriter::DONE_PARTIAL
+                    ) {


This and the above code is a safety check for if an archive has a done like flag but DONE_PARTIAL value. In case of problematic data like this, we just treat it like another partial archive.

diosmosis · 2021-04-09T05:50:24Z

core/DataAccess/ArchiveWriter.php

+                'exception' => $ex,
+            ]);
+        }
+    }


Adding a log just in case there is some other way to create a partial archive w/o a plugin done flag.

diosmosis · 2021-04-09T05:51:13Z

FYI @tsteur

tsteur · 2021-04-11T21:16:28Z

@diosmosis can you have a look at the failing test they might be related to this? If they fail because of this PR I will have another look after fixing it

diosmosis · 2021-04-12T00:01:14Z

@tsteur relevant tests should pass now

tsteur

haven't tested anything but left few comments just to check. Looked through the code and wasn't sure if it could maybe result in some wrong archiving like when we mark something as not partial but we actually only want to archive a plugin.

tsteur · 2021-04-12T00:56:22Z

core/ArchiveProcessor/Loader.php

@@ -105,6 +105,7 @@ public function prepareArchive($pluginName)
    private function prepareArchiveImpl($pluginName)
    {
        $this->params->setRequestedPlugin($pluginName);
+        $this->params->setIsPartialArchive(false);


@diosmosis in line 113 should it set $this->params->setArchiveOnlyReport($requestedReport) or maybe setArchiveOnlyReport() should set setIsPartialArchive automatically?

tsteur · 2021-04-12T00:57:20Z

core/ArchiveProcessor/Loader.php

@@ -105,6 +105,7 @@ public function prepareArchive($pluginName)
    private function prepareArchiveImpl($pluginName)
    {
        $this->params->setRequestedPlugin($pluginName);
+        $this->params->setIsPartialArchive(false);


do we ever need to reset the original/previously set IsPartialArchive value maybe? haven't checked yet where it's reused

tsteur · 2021-04-12T00:58:15Z

core/ArchiveProcessor/Loader.php

@@ -184,13 +184,18 @@ protected function prepareCoreMetricsArchive($visits, $visitsConverted)

            $this->params->setRequestedPlugin('VisitsSummary');
            $this->params->setArchiveOnlyReport(null);
+            $this->params->setIsPartialArchive(false);


would we need to back up the previously set value and restore it below in line 198?

diosmosis · 2021-04-12T01:07:39Z

in line 113 should it set $this->params->setArchiveOnlyReport($requestedReport) or maybe setArchiveOnlyReport() should set setIsPartialArchive automatically?

I don't think it's technically necessary to set it, but it makes sense to be thorough. setArchiveOnlyReport() shouldn't call setIsPartialArchive(), that's called specifically by Archiver's when they handle the requestedReport parameter, letting ArchiveWriter know it only archived a single report.

I thought about unsetting it in that method, but we never re-use Parameters objects anywhere else (and shouldn't in normal uses), so it seemed like it could cause strange side effects.

do we ever need to reset the original/previously set IsPartialArchive value maybe? haven't checked yet where it's reused

In that specific line of code, no, because that's the start of the prepareArchive() method. In fact in that line of code we don't need to call the method there. I'll remove that.

would we need to back up the previously set value and restore it below in line 198?

Don't need to since we archive VisitsSummary before other plugins. We could do it, though, just in case something changes in the future.

tsteur · 2021-04-12T01:28:15Z

Don't need to since we archive VisitsSummary before other plugins. We could do it, though, just in case something changes in the future.

I reckon that might be useful just in case

diosmosis · 2021-04-12T02:58:29Z

@tsteur applied pr feedback

tsteur · 2021-04-12T03:29:55Z

fyi @diosmosis there are now some merge conflicts from merging the other PR

diosmosis · 2021-04-12T03:31:19Z

@tsteur 👍 just fixed, we'll see if the build passes

tsteur · 2021-04-12T03:52:51Z

btw is this one now no longer needed 706621b#diff-3ad7e38a2ff2d49f9ac9f268bf0ce70db9b1a924be38e5802816acc0ae343d32L108 ?

diosmosis · 2021-04-12T04:02:16Z

@tsteur it was never really needed, since Loader is given a new Parameters instance just before using it. So it's already in a clean state.

Add new test for partial archiving edge case, add two safety/sanity c…

1f7486a

…hecks for partial archives, fix a couple issues that arise when archiving a multi-period partial archive that has to initiate archiving for a child archive and reuse archives for a child archive.

diosmosis added the Needs Review PRs that need a code review label Apr 9, 2021

diosmosis added this to the 4.3.0 milestone Apr 9, 2021

diosmosis added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 9, 2021

diosmosis commented Apr 9, 2021

View reviewed changes

diosmosis mentioned this pull request Apr 9, 2021

Regression: some daily reports disappear from UI, and weekly reports are corrupted #17428

Closed

remove unneeded test and make existing test more trustworthy

b7c9c6d

tsteur reviewed Apr 12, 2021

View reviewed changes

diosmosis added 2 commits April 11, 2021 19:54

Merge branch '4.x-dev' into partial-all-safety-check

44cf777

apply review feedback

706621b

Merge branch '4.x-dev' into partial-all-safety-check

e39adbb

diosmosis merged commit 0e6a6ea into 4.x-dev Apr 12, 2021

diosmosis deleted the partial-all-safety-check branch April 12, 2021 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for specific case when partial archives have to initiate archiving for child archives #17439

Fixes for specific case when partial archives have to initiate archiving for child archives #17439

diosmosis commented Apr 9, 2021

diosmosis Apr 9, 2021 •

edited

tsteur Apr 12, 2021

tsteur Apr 12, 2021

diosmosis Apr 9, 2021

diosmosis Apr 9, 2021

diosmosis Apr 9, 2021

diosmosis Apr 9, 2021

diosmosis Apr 9, 2021

diosmosis Apr 9, 2021

diosmosis commented Apr 9, 2021

tsteur commented Apr 11, 2021

diosmosis commented Apr 12, 2021

tsteur left a comment

tsteur Apr 12, 2021

tsteur Apr 12, 2021

tsteur Apr 12, 2021

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

Fixes for specific case when partial archives have to initiate archiving for child archives #17439

Fixes for specific case when partial archives have to initiate archiving for child archives #17439

Conversation

diosmosis commented Apr 9, 2021

Description:

Review

diosmosis Apr 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Apr 9, 2021

tsteur commented Apr 11, 2021

diosmosis commented Apr 12, 2021

tsteur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

tsteur commented Apr 12, 2021

diosmosis commented Apr 12, 2021

diosmosis Apr 9, 2021 •

edited