https://github.com/matomo-org/matomo/pull/17439 - Mouve

Site d'origine

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.

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

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

Merged
merged 5 commits into from Apr 12, 2021

Conversation

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 9, 2021

Description:

Refs #17428

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

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed
…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 diosmosis added this to the 4.3.0 milestone Apr 9, 2021
@@ -105,6 +105,7 @@ public function prepareArchive($pluginName)
private function prepareArchiveImpl($pluginName)
{
$this->params->setRequestedPlugin($pluginName);
$this->params->setIsPartialArchive(false);

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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.

This comment has been minimized.

@tsteur

tsteur Apr 12, 2021
Member

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

This comment has been minimized.

@tsteur

tsteur Apr 12, 2021
Member

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

@@ -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()

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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.

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

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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.)

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

public static function isForceArchivingSinglePlugin()
public static function isRequestingToAndAbleToForceArchiveSinglePlugin()

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

Renamed to show it's true function more clearly.

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

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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).
// 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 comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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.

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

This comment has been minimized.

@diosmosis

diosmosis Apr 9, 2021
Author Member

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

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 9, 2021

FYI @tsteur

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 11, 2021

@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
Copy link
Member Author

@diosmosis diosmosis commented Apr 12, 2021

@tsteur relevant tests should pass now

Copy link
Member

@tsteur tsteur left a comment

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.

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

This comment has been minimized.

@tsteur

tsteur Apr 12, 2021
Member

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

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

This comment has been minimized.

@tsteur

tsteur Apr 12, 2021
Member

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

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

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

This comment has been minimized.

@tsteur

tsteur Apr 12, 2021
Member

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

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 12, 2021

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
Copy link
Member

@tsteur tsteur commented Apr 12, 2021

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
Copy link
Member Author

@diosmosis diosmosis commented Apr 12, 2021

@tsteur applied pr feedback

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 12, 2021

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

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 12, 2021

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

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 12, 2021

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 12, 2021

@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.

@diosmosis diosmosis merged commit 0e6a6ea into 4.x-dev Apr 12, 2021
1 of 3 checks passed
1 of 3 checks passed
@github-actions
PHPCS
Details
@travis-ci
Travis CI - Branch Build Failed
Details
@travis-ci
Travis CI - Pull Request Build Failed
Details
@diosmosis diosmosis deleted the partial-all-safety-check branch Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants

level de log mis à 1

Récupération du document https://github.com/matomo-org/matomo/pull/17439.

Récupération de l'URL https://github.com/matomo-org/matomo/pull/17439...

Raccourcis

Commandes

Fermer