If archive_invalidations is in inconsistent state, fix as getting next archive to process. #16886

diosmosis · 2020-12-05T02:37:49Z

Description:

In case the archive_invalidations table ends up in an inconsistent state, make sure we process invalidations higher than what we find.

cc @tsteur

Review

Functional review done
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

…t archive to process.

tsteur · 2020-12-06T20:58:10Z

core/CronArchive/QueueConsumer.php

+            $bind[] = $archiveToProcess['report'];
+        }
+
+        $sql = "SELECT DISTINCT period FROM `$table` WHERE idsite = ? AND name = ? AND period > ? AND ? >= date1 AND date2 >= ? $reportClause";


btw is the distinct actually needed?

could do an array_unique as well, was trying to reduce the amount of data selected since there could potentially be a lot of invalidations for a site

all good should be fine to keep distinct for now

tsteur · 2020-12-06T20:59:14Z

@sgiehl be great if you could review this tomorrow. We'd probably need to merge this at the same time as #16843 to fully avoid too many duplicates.

tsteur · 2020-12-07T23:22:08Z

core/CronArchive/QueueConsumer.php

@@ -278,6 +280,8 @@ public function getNextArchivesToProcess()

            $this->logger->debug("Processing invalidation: $invalidationDesc.");

+            $this->repairInvalidationsIfNeeded($invalidatedArchive);


@diosmosis any thought about doing this in CronArchive::launchArchivingFor() after it finished successfully? Technically we don't need to schedule the higher period if it's not successful and also it could prevent a race condition where the day archive takes many hours and by the time it's finished the higher period does not exist anymore if I see this right?

any thought about doing this in CronArchive::launchArchivingFor() after it finished successfully?

I figured it would be better to do it here, since when we invalidate through ArchiveInvalidator, we always add all higher periods. So at the start, we always have the whole list of invalidations.

Technically we don't need to schedule the higher period if it's not successful and also it could prevent a race condition where the day archive takes many hours and by the time it's finished the higher period does not exist anymore if I see this right?

The only case where the higher period would not exist anymore is if another archiver was processing the same site and picked it up, but this could happen w/o this repairing logic... In that case we'd have to do the intersecting period logic w/ a query on archive_invalidations.

So at the start, we always have the whole list of invalidations.

you mean then we can batch insert it? I reckon ideally we do it after finishing the archive just to make sure there can't be a race condition or any other issues I would say. It be fine if it's just one insert at a time I would say (we could always tweak when needed)

@tsteur no I mean, in the invalidations table, normally we have all the higher periods before we start archiving, so doing the repair logic here mimics that behavior.

We can still do it after finishing an archive (though there will be less test coverage then).

Be great to do it after finishing the archive. Could keep the method and just call it from CronArchive instead of QueueConsumer @diosmosis ? It's really just to avoid random edge cases etc which will happen

tsteur · 2020-12-07T23:24:19Z

core/CronArchive/QueueConsumer.php

+            }
+
+            // archive is for week that is over two months, we don't need to care about the month
+            if ($label == 'month'


not sure why we don't need to care about this case? There might be still issues eg when someone imports data in the past etc?

It's because the week will be on two months, so it won't actually be used in archiving either of the parent months.

There could be the case where there was a day that was archived, then for some reason the month/year disappear, and it's just the week. Then I guess we would need new month/year re-archives, but wouldn't be able to.

It seemed a waste to have to re-archive both months, but I can add it if necessary.

Got it now 👍 We only need to archive the correct month

diosmosis · 2020-12-08T02:03:13Z

@tsteur updated, there are still tests for it, but some of the effects seen in the queue consumer tests are no longer there

tsteur · 2020-12-08T02:54:35Z

core/CronArchive.php

+
+        $higherPeriods = Db::fetchAll($sql, $bind);
+        $higherPeriods = array_column($higherPeriods, 'period');
+        $higherPeriods = array_flip($higherPeriods);


not important but could have done instead below an in_array($id, $higherPeriods) . not too important though

sure, thought this was a bit faster, but for an array w/ 3 items at most, probably not

tsteur · 2020-12-08T02:55:31Z

core/CronArchive.php

+                'date1' => $period->getDateStart()->getDatetime(),
+                'date2' => $period->getDateEnd()->getDatetime(),
+                'period' => $id,
+                'ts_invalidated' => $archiveToProcess['ts_invalidated'],


should this have maybe the current date? Just to be more clear when it was inserted. Or is it so it's included to be archived in this archive run maybe? (as we maybe do somewhere and ts_invalidated < idSiteStartTime)

Yes, it's done that way so we process it in this batch. (getNextInvalidatedArchive does the ts_invalidated < idSiteStartTime)

tsteur · 2020-12-08T02:58:06Z

core/CronArchive.php

+            $bind[] = $archiveToProcess['report'];
+        }
+
+        $sql = "SELECT DISTINCT period FROM `$table` WHERE idsite = ? AND name = ? AND period > ? AND ? >= date1 AND date2 >= ? $reportClause";


wondering. Do we here also need to include and status = 0?

tsteur

@diosmosis worked for me. Only need to maybe have a look at https://github.com/matomo-org/matomo/pull/16886/files#r537995820 as maybe the query needs to have to check status

tsteur · 2020-12-08T03:00:00Z

Otherwise good to merge once the other comment re the where clause is resolved and tests pass

diosmosis added 2 commits December 4, 2020 18:34

If archive_invalidations is in inconsistent state, fix as getting nex…

959f0aa

…t archive to process.

remove in progress invalidation block

6151be4

diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Dec 5, 2020

diosmosis added this to the 4.0.3 milestone Dec 5, 2020

tsteur reviewed Dec 6, 2020

View reviewed changes

tsteur requested a review from sgiehl December 6, 2020 20:58

tsteur requested changes Dec 7, 2020

View reviewed changes

diosmosis added 2 commits December 7, 2020 17:34

move repair logic to after archives launched

1790e01

Add debug log for reparing invalidation table

991d19b

Merge branch '4.x-dev' into fix-inconsistent-table

77798a6

tsteur reviewed Dec 8, 2020

View reviewed changes

diosmosis added 2 commits December 7, 2020 19:50

apply review feedback

9c3a48b

fix test

8408363

diosmosis merged commit dc3f9db into 4.x-dev Dec 8, 2020

diosmosis deleted the fix-inconsistent-table branch December 8, 2020 05:07

bx80 mentioned this pull request Jul 26, 2022

Fix for current year not processed for new custom reports #19569

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If archive_invalidations is in inconsistent state, fix as getting next archive to process. #16886

If archive_invalidations is in inconsistent state, fix as getting next archive to process. #16886

diosmosis commented Dec 5, 2020

tsteur Dec 6, 2020

diosmosis Dec 6, 2020

tsteur Dec 6, 2020

tsteur commented Dec 6, 2020

tsteur Dec 7, 2020

diosmosis Dec 7, 2020

tsteur Dec 8, 2020

diosmosis Dec 8, 2020

tsteur Dec 8, 2020

tsteur Dec 7, 2020

diosmosis Dec 7, 2020

tsteur Dec 8, 2020

diosmosis commented Dec 8, 2020

tsteur Dec 8, 2020

diosmosis Dec 8, 2020

tsteur Dec 8, 2020

diosmosis Dec 8, 2020

tsteur Dec 8, 2020

diosmosis Dec 8, 2020

tsteur left a comment

tsteur commented Dec 8, 2020

		@@ -278,6 +280,8 @@ public function getNextArchivesToProcess()

		$this->logger->debug("Processing invalidation: $invalidationDesc.");

		$this->repairInvalidationsIfNeeded($invalidatedArchive);

If archive_invalidations is in inconsistent state, fix as getting next archive to process. #16886

If archive_invalidations is in inconsistent state, fix as getting next archive to process. #16886

Conversation

diosmosis commented Dec 5, 2020

Description:

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Dec 6, 2020

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 Dec 8, 2020

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

tsteur left a comment

Choose a reason for hiding this comment

tsteur commented Dec 8, 2020