Add mechanism to invalidate dependent segment archives

[michalkleiner]

Description:

Add new Archiver method getDependentSegments to return a list of dependent segments, and update the existing getDependentSegmentsToArchive method to use it by default while being backward compatible (see e.g. Goals or Media Analytics archivers).

Add a mechanism to invalidate dependent segments to the ArchiveInvalidator class.

Adds code to invalidate dependent segment archives right before they should be archived (again).

Fixes #18772
Ref. DEV-14109

Massive thanks to @sgiehl for his help, guidance and a productive pairing session!

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
• Wording review done
• 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

[tsteur]

fyi just came across this PR randomly and saw the many code changes. You might have already thought about it, but wanted to mention it in case we haven't.

When we're archiving and processDependentArchive() is called, I believe we can be certain that the dependent archive has to be reprocessed too. It means in those cases we can maybe force to invalidate. We maybe want to test a change like below.

diff --git a/core/ArchiveProcessor.php b/core/ArchiveProcessor.php
index 19ae9c992a..7b1d06de8d 100644
--- a/core/ArchiveProcessor.php
+++ b/core/ArchiveProcessor.php
@@ -771,6 +771,10 @@ class ArchiveProcessor
 
         self::$isRootArchivingRequest = false;
         try {
+            $invalidator = StaticContainer::get('Piwik\Archive\ArchiveInvalidator');
+            $invalidator->markArchivesInvalidated($params->getIdSites(), [date...], $newSegment,
+                            $params->getPeriod()->getLabel() != 'range', $forceInvalidateNonexistentRanges, $plugin);
+
             $parameters = new ArchiveProcessor\Parameters($params->getSite(), $params->getPeriod(), $newSegment);
             $parameters->onlyArchiveRequestedPlugin();

Might be worth a test if we haven't checked that yet.

I believe this would also avoid any possible race conditions should multiple archivers be running concurrently etc.

[sgiehl]

Thanks @tsteur for pointing towards that. Doing the invalidation only in case it should be archived again makes everything a bit easier I guess. I will try to adjust the code and see if everything still works as expected.

[sgiehl]

Ok. So only adding the invalidation right before also doesn't work as expected. While the final result is fine, it actually will archive the dependent segment twice. The invalidation record, which is created during invalidation, isn't removed after processing the dependent segment, causing the archiver to invalidate the data right away and archive it again 🙈

Edit: Actually the data isn't processed again, as the segment hashes in the invalidation are not stored anywhere. Therefor the archiver will produce a lot of warnings as the segments can't be found. I'll add code to avoid adding invalidations in that case.

[sgiehl]

This PR should now be ready for a final review. I've changed the approach to Thomas suggestion, which I hadn't thought about before.
I've also added a couple of comments to make it easier to understand certain additional required changes.

[sgiehl]

This is a small fix to the plugin detection. When a name is provided, but doesn't contain a ., it is actually the plugin name only. So we should check that correctly.

[sgiehl]

This change is caused by invalidating the VisitsSummary archive, before archiving the goal metrics. As the goal archiver is using data from like nb_visits, it was using outdated data before.

[sgiehl]

This is caused by some weird behavior of archiving. When archiving is triggered for anything, the first thing that is done is checking if core metrics (aka VisitsSummary) needs to be archived. This is done by checking if there is an archive that contains the visits metric. If this isn't the case VisitsSummary will be archived.
In case we have an archive that doesn't contain any visits, the archive will only contain a done flag, but no other metrics. This causes the archiving to be triggered again, which will create a new (empty) archive, while removing the previous one.
As archiving dependent segments first triggers archiving VisitsSummary, it creates an empty archive. Afterwards when archiving the Goals plugin it will archive VisitsSummary again, as the previous one is empty. Which then causes this missing archive id.

[michalkleiner]

Maybe add some of the explanation from the GH comment as part of the code comment?

[sgiehl]

This is a small fix, which caused multiple archives to exist for the same parameters when they were created in the same second. (See update test)

[sgiehl]

Those archives are now correctly removed even if they are created in the same second.