Delayed rearchiving #16702

diosmosis · 2020-11-11T06:48:09Z

Description:

Schedule rearchiving calls in a list and apply later in core:archive. 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

…_invalidations do not slow UI

sgiehl

Guess the advantage of this changes is that it won't take longer to perform any action that created dozens of invalidations before, as those records are now created while archiving, right?

sgiehl · 2020-11-11T15:39:22Z

core/Archive/ArchiveInvalidator.php

     *
     * @param int|int[]|'all' $idSites
     * @param string $pluginName
     * @param string|null $report
     * @param Date|null $startDate
     */
-    public function reArchiveReportSafely($idSites, string $pluginName, string $report = null, Date $startDate = null)
+    public function scheduleReArchiving($idSites, string $pluginName, string $report = null, Date $startDate = null)


Guess we need to adjust that in the plugins as well once that is merged

sgiehl · 2020-11-11T16:05:49Z

core/Archive/ArchiveInvalidator.php

+                    'ex' => $ex,
+                ]);
+            } finally {
+                $reArchiveList->remove([$entry]);


Does it make sense to remove the record even if it failed to create the invalidation? Shouldn't we keep it and try to add it another time?

No preference from my side here as I reckon usually it shouldn't fail? if there may be some reasons why it might happen could also keep the entry and retry later

It should only fail if the input is broken for some reason, this is mostly to replicate the old behavior where if it failed for some reason, invalidations wouldn't be added.

Now that the entries are json encoded, that needs to be done for the removal as well. Otherwise it won't be removed at all. Alternatively you could also use another variable for the decoded entry...

Suggested change

$reArchiveList->remove([$entry]);

$reArchiveList->remove([json_encode($entry)]);

tsteur · 2020-11-11T19:52:28Z

core/Archive/ArchiveInvalidator.php

+            try {
+                list($idSites, $pluginName, $report, $startDateTs) = $entry;
+
+                $this->reArchiveReport($idSites, $pluginName, $report, Date::factory($startDateTs));


maybe we should only create date instance if startDateTs is not empty cause otherwise it might result in an error or creates a 'now' date or something else unexpected?

tsteur · 2020-11-11T19:54:05Z

core/Archive/ArchiveInvalidator.php

    {
        try {
-            $this->reArchiveReport($idSites, $pluginName, $report, $startDate);
+            $reArchiveList = new ReArchiveList($this->logger);
+            $reArchiveList->add([$idSites, $pluginName, $report, $startDate ? $startDate->getTimestamp() : null]);


Personally would prefer if we used an array like ['idsites' => $idSites, ...] this would allow us to quite easily extend or change the structure in the future if/when needed. and makes it maybe also more clear what each array item represents? Not too important though. It be mostly an issue if we eg ever removed for example an argument and then would need to do things like [$idSites, $pluginName, null, null, $newArgument] or something

tsteur · 2020-11-11T19:54:47Z

core/CronArchive/ReArchiveList.php

+use Piwik\Concurrency\DistributedList;
+use Psr\Log\LoggerInterface;
+
+class ReArchiveList extends DistributedList


it's great this was so easy to do and that we have this distributedlist already!

core/CronArchive.php

tsteur

@diosmosis ideally we also adjust removeInvalidationsSafely to additionally remove any entry from the list that matches the given pluginName (and maybe idsites)? just to prevent a race condition when quickly activating and deactivating would cause the invalidations to be added after the plugin was deactivated. although maybe these invalidations would be ignored anyway when the plugin is no longer active?

diosmosis · 2020-11-11T23:06:48Z

ideally we also adjust removeInvalidationsSafely to additionally remove any entry from the list that matches the given pluginName (and maybe idsites)?

👍

diosmosis · 2020-11-12T03:06:53Z

@tsteur Applied pr fixes.

tsteur · 2020-11-12T03:16:43Z

I'll be mostly in meetings tmrw. @sgiehl could you test this tomorrow? and then we adjust the plugins that use this in activate/deactivate plugin

sgiehl · 2020-11-12T08:45:43Z

core/Archive/ArchiveInvalidator.php

+        foreach ($entries as $index => $entry) {
+            $entry = @json_decode($entry, true);
+            if (empty($entry)) {
+                continue;


wouldn't it make sense to always remove entries that can't be decoded? Otherwise they might kept in the list forever

sgiehl · 2020-11-12T11:17:40Z

core/Archive/ArchiveInvalidator.php

+                    $entry['idSites'],
+                    $entry['pluginName'],
+                    $entry['report'],
+                    !empty($entry['startDate']) ? Date::factory($entry['startDate']) : null


should we maybe cast $entry['startDate'] to integer before passing it to the factory. It should normally already be an integer, but if it would be a string (containing a number), the factory will throw an exception

sgiehl · 2020-11-12T11:18:08Z

tested that locally with adjusted Cohorts PR. Besides the comments I've added and #16705 it seems to work as expected.

tsteur · 2020-11-13T02:33:38Z

👍 @diosmosis so seems good to merge once tests pass?

sgiehl · 2020-11-13T07:39:50Z

Guess you missed that comment: #16702 (comment)
Will create a PR to fix that

diosmosis · 2020-11-14T03:48:23Z

Guess you missed that comment: #16702 (comment)

yes, missed it, the tab that had this github page didn't update I guess

diosmosis added 3 commits November 10, 2020 22:30

Schedule rearchiving so potentially long running inserts into archive…

7144421

…_invalidations do not slow UI

make sure to apply rearchiving

688dda1

add file

dfab027

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 Nov 11, 2020

diosmosis added this to the 4.0.0-RC milestone Nov 11, 2020

sgiehl requested changes Nov 11, 2020

View reviewed changes

tsteur reviewed Nov 11, 2020

View reviewed changes

core/CronArchive.php Outdated Show resolved Hide resolved

tsteur reviewed Nov 11, 2020

View reviewed changes

apply review feedback, add tests + get to pass

94d27ca

sgiehl reviewed Nov 12, 2020

View reviewed changes

sgiehl mentioned this pull request Nov 12, 2020

Fix methods to get plugin de-/activation time. #16705

Merged

9 tasks

sgiehl reviewed Nov 12, 2020

View reviewed changes

diosmosis added 2 commits November 12, 2020 21:42

Merge branch '4.x-dev' into delayed-rearchiving

5b5f28f

apply review feedback and fix tests

2ae0a38

diosmosis force-pushed the delayed-rearchiving branch from a099ff8 to 2ae0a38 Compare November 13, 2020 05:52

diosmosis merged commit 9a9981d into 4.x-dev Nov 13, 2020

diosmosis deleted the delayed-rearchiving branch November 13, 2020 06:32

sgiehl mentioned this pull request Nov 13, 2020

Fix removal of rearchiving records #16711

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delayed rearchiving #16702

Delayed rearchiving #16702

diosmosis commented Nov 11, 2020

sgiehl left a comment

sgiehl Nov 11, 2020

sgiehl Nov 11, 2020

tsteur Nov 11, 2020

diosmosis Nov 11, 2020

sgiehl Nov 12, 2020

tsteur Nov 11, 2020

diosmosis Nov 11, 2020

tsteur Nov 11, 2020

diosmosis Nov 11, 2020

tsteur Nov 11, 2020

tsteur left a comment

diosmosis commented Nov 11, 2020

diosmosis commented Nov 12, 2020

tsteur commented Nov 12, 2020

sgiehl Nov 12, 2020

diosmosis Nov 13, 2020

sgiehl Nov 12, 2020

diosmosis Nov 13, 2020

sgiehl commented Nov 12, 2020

tsteur commented Nov 13, 2020

sgiehl commented Nov 13, 2020

diosmosis commented Nov 14, 2020

	$reArchiveList->remove([$entry]);
	$reArchiveList->remove([json_encode($entry)]);

Delayed rearchiving #16702

Delayed rearchiving #16702

Conversation

diosmosis commented Nov 11, 2020

Description:

Review

sgiehl 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

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

tsteur left a comment

Choose a reason for hiding this comment

diosmosis commented Nov 11, 2020

diosmosis commented Nov 12, 2020

tsteur commented Nov 12, 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

sgiehl commented Nov 12, 2020

tsteur commented Nov 13, 2020

sgiehl commented Nov 13, 2020

diosmosis commented Nov 14, 2020