fix concurrency archiving overview goals #18243

peterhashair · 2021-10-29T12:17:00Z

Description:

Fixes: #18088
Fix concurrency archiving overview goals with multiple ajax requests. Used a flock lock on the api request. Not the best solution, but should reduce the number of multiple archiving.

Review

peterhashair · 2021-10-31T00:38:19Z

@tsteur I think that PR fixed the concurrency for the goal overview archive

add flock lock on api end point

tsteur · 2021-11-01T21:35:03Z

btw @peterhashair it be great if any lock would be only used if Rules::isArchivingEnabledFor() && !SettingsServer::isArchivePhpTriggered(); because only then we will actually need it.

That means the lock will be only used if browser archiving is used or when a range is being requested. This way we avoid the overhead of locks when it's doing the regular archiving using cron as the cli archiver already pretty much makes sure we won't archive the same date concurrently.

Also ideally the solution would automatically work for all reports if possible. If we only fix it eg in Goals then we could as well just use the JS fix to only request one widget instead of all of them

add concurreny lock

remove extra lock check

update lock

update load test

add lock test

add mysql lock

upload lock

peterhashair · 2021-11-03T23:06:48Z

@tsteur I locked Rules::isArchivingEnabledFor() && !SettingsServer::isArchivePhpTriggered(); those two, it still has 1 duplicated after I tried some task 10 times. Is there any more places needs to lock?

matomo/core/ArchiveProcessor/Loader.php

Lines 136 to 171 in 0475bc2

    
           try { 
        
               if (SettingsServer::isArchivePhpTriggered()) { 
        
                   $requestedReport = Common::getRequestVar('requestedReport', '', 'string'); 
        
                   if (!empty($requestedReport)) { 
        
                       $this->params->setArchiveOnlyReport($requestedReport); 
        
                   } 
        
               } 
        
               // invalidate existing archives before we start archiving in case data was tracked in the past. if the archive is 
        
               // made invalid, we will correctly re-archive below. 
        
               if ($this->invalidateBeforeArchiving 
        
                 && Rules::isBrowserTriggerEnabled() 
        
               ) { 
        
                   $this->invalidatedReportsIfNeeded(); 
        
               } 
        
               if (SettingsServer::isArchivePhpTriggered()) { 
        
                   $this->logger->info("initiating archiving via core:archive for " . $this->params); 
        
               } 
        
               //pick select 
        
               list($visits, $visitsConverted) = $this->loadArchiveData(); 
        
               if ($this->quickReturn) { 
        
                   return [$visits, $visitsConverted]; 
        
               } 
        
               //insert data 
        
               list($visits, $visitsConverted) = $this->prepareCoreMetricsArchive($visits, $visitsConverted); 
        
               list($idArchive, $visits) = $this->prepareAllPluginsArchive($visits, $visitsConverted); 
        
               if ($this->isThereSomeVisits($visits) || PluginsArchiver::doesAnyPluginArchiveWithoutVisits()) { 
        
                   return [[$idArchive], $visits]; 
        
               } 
        
           } catch (\Exception $e) {

tsteur · 2021-11-03T23:58:55Z

@peterhashair In chat the other day I mentioned we need to should only lock when if ($this->params->isRootArchiveRequest() && !SettingsServer::isArchivePhpTriggered()) {. Maybe try if that helps (we'd definitely need a check to only lock in root archive request and not archive if the archiving is triggered).

If then there's still one duplicate that should be fine. It's also possible that the duplicate goes away once #18255 is merged

…chive

sgiehl

Left a couple of comments

core/ArchiveProcessor/LoaderLock.php

plugins/Goals/API.php

core/Concurrency/Lock.php

core/ArchiveProcessor/LoaderLock.php

sgiehl · 2021-11-09T17:49:01Z

core/ArchiveProcessor/LoaderLock.php

+    {
+        $row = Db::fetchOne('SELECT IS_FREE_LOCK(?);', array($this->id));
+
+        if ($row[0] === 1) {


That won't work for all DB adapters. PDO and MySQLi differ in return types. Might be possible that a string is returned instead, so be good to cast that.
Btw. If you write some simple tests for that, you can see on travis if it works for both adapters.

core/ArchiveProcessor/Loader.php

Co-authored-by: Stefan Giehl <stefan@matomo.org>

core/ArchiveProcessor/LoaderLock.php

tsteur · 2021-11-10T00:30:33Z

core/ArchiveProcessor/LoaderLock.php

+/**
+ * This class uses PluginsArchiver class to trigger data aggregation and create archives.
+ */
+class LoaderLock


@peterhashair can we write tests for this class to make sure it works?

@tsteur add a test, simple one, let me know if that doesn't meet the requirements

@peterhashair can you move this test into a LoaderLockTest.php? For each class/file we also create a different test class.

What you additionally want to test is that

Create another lock with a different idea, check that the initially created lock doesn't also lock this lock with different ID. To make sure it only locks the specific ID.

A test where you enter a long ID with > 64 characters

Call "unlock" when it's not locked yet

Are there other edge cases that can be tested?

@tsteur update it, but it seems like Travis number 7 angular test is started to fail. https://app.travis-ci.com/github/matomo-org/matomo/jobs/547651253. There may be a regression somewhere

core/ArchiveProcessor/Loader.php

add locker tests

…chive

remove unsed functions

group into functions

update screen shot to question 1

update wrong place

update loaderlock test

tsteur

@peterhashair this worked nicely for me. I left a few more comments to look at.

There is also a test failing that needs to be fixed: https://app.travis-ci.com/github/matomo-org/matomo/jobs/547651249#L787

The failing angular test should be unrelated to this change.

I expect this to be the last iteration and will do a final test again after these changes.

core/ArchiveProcessor/Loader.php

tests/PHPUnit/Integration/ArchiveProcessor/LoaderLockTest.php

tsteur · 2021-11-11T20:46:18Z

tests/PHPUnit/Integration/ArchiveProcessor/LoaderLockTest.php

+
+        $lock = new LoaderLock($lockId);
+        $check = strlen($lock->getId())<64;
+        $this->assertEquals($check,1);


just fyi (no change required).

Here you can use $this->assertTrue(strlen($lock->getId())<64); or even better $this->assertLessThanOrEqual(64,strlen($lock->getId()));

actually in this case you would even want to check that $this->assertSame(64,strlen($lock->getId()));

because the test would otherwise pass even if an empty ID was returned so we can't actually check that the logic works.

The very best solution would be to not produce a random string, but created a fixed string like $lockId = str_pad('test', 128, '1');

and then test the exact returned value for the lock id like

$this->assertSame(' the generated lock id', $lock->getId())

This way you make sure that the method works as intended. This should be actually changed so we make sure it shortens the ID correctly as expected.

core/ArchiveProcessor/Loader.php

core/ArchiveProcessor/LoaderLock.php

remove quickReturn, update tests

update loader

…chive

tsteur · 2021-11-15T02:34:55Z

@peterhashair I've made few tweaks like:

removing requires MySQL 5.6 comment as it's been there at least in 4.1 http://download.nust.na/pub6/mysql/doc/refman/4.1/en/miscellaneous-functions.html . Otherwise we would have needed to catch potential exceptions etc. Or did you maybe mean some other reason why it required MySQL 5.6 for the locks?
When the lockID is < 64 characters then we don't hash the ID. I mentioned this in the comment as there's not always a need to hash it
The log "initiating archiving via core:archive for..." wasn't always logged and moved it now into insertArchiveData.

I tested it again and worked nicely. If the tests pass then we can merge 🎉 . Made these changes quickly myself so we can merge it before the release to avoid another round of changes and reviews.

add flock lock on api end point

52b9dfe

add flock lock on api end point

peterhashair force-pushed the m-18088-mutiple-archive branch from 41d9aaf to 52b9dfe Compare November 1, 2021 10:46

Peter Zhang and others added 8 commits November 2, 2021 11:09

add concurreny lock

5150901

add concurreny lock

remove extra lock check

48f6d1b

remove extra lock check

update lock

2101756

update lock

Update Loader.php

3925507

update load test

add lock test

0a35826

add lock test

add mysql lock

084f9b8

add mysql lock

upload lock

8b9ba19

upload lock

Merge branch '4.x-dev' into m-18088-mutiple-archive

0475bc2

Peter Zhang added 3 commits November 5, 2021 16:41

Merge remote-tracking branch 'origin/4.x-dev' into m-18088-mutiple-ar…

3dee45e

…chive

Merge remote-tracking branch 'origin/4.x-dev' into m-18088-mutiple-ar…

a9f44a5

…chive

update load

9cc4b42

peterhashair marked this pull request as ready for review November 7, 2021 21:02

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

peterhashair added this to the 4.6.0 milestone Nov 7, 2021

peterhashair assigned peterhashair and unassigned peterhashair Nov 7, 2021

peterhashair added the Needs Review PRs that need a code review label Nov 7, 2021

sgiehl requested changes Nov 9, 2021

View reviewed changes

sgiehl removed the Needs Review PRs that need a code review label Nov 9, 2021

Update core/ArchiveProcessor/LoaderLock.php

ab95e05

Co-authored-by: Stefan Giehl <stefan@matomo.org>

tsteur requested changes Nov 10, 2021

View reviewed changes

Peter Zhang added 3 commits November 10, 2021 16:17

add locker tests

9153995

add locker tests

upload

5a8520b

Merge remote-tracking branch 'origin/4.x-dev' into m-18088-mutiple-ar…

084512a

…chive

Peter Zhang added 5 commits November 10, 2021 17:08

Update Lock.php

ad51ce6

remove unsed functions

revert composer no change on those files

54c002b

group into functions

Create FeedbackQuestion_feedback_popup.png

b22bdcb

update screen shot to question 1

Delete FeedbackQuestion_feedback_popup.png

8a78cac

update wrong place

update loaderlock test

4b8c42a

update loaderlock test

tsteur requested changes Nov 11, 2021

View reviewed changes

Peter Zhang added 4 commits November 12, 2021 12:27

remove quickReturn, update tests

1713d41

remove quickReturn, update tests

update loader

75e98b2

update loader

update loader

c83ba06

update loader

Merge remote-tracking branch 'origin/4.x-dev' into m-18088-mutiple-ar…

4a2b051

…chive

peterhashair added the Needs Review PRs that need a code review label Nov 15, 2021

tsteur added 2 commits November 15, 2021 15:18

apply some review feedback

d8119a0

add comment about requiring 5.6 as it was already there in 4.1

c620840

tsteur merged commit ae761a2 into 4.x-dev Nov 15, 2021

tsteur deleted the m-18088-mutiple-archive branch November 15, 2021 03:31

tsteur mentioned this pull request Nov 15, 2021

Ensure that the archive algorithm cannot be triggered multiple times for a same site/period/segment #8444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix concurrency archiving overview goals #18243

fix concurrency archiving overview goals #18243

peterhashair commented Oct 29, 2021 •

edited

peterhashair commented Oct 31, 2021

tsteur commented Nov 1, 2021

peterhashair commented Nov 3, 2021 •

edited

tsteur commented Nov 3, 2021

sgiehl left a comment

sgiehl Nov 9, 2021

tsteur Nov 10, 2021

peterhashair Nov 11, 2021

tsteur Nov 11, 2021

peterhashair Nov 11, 2021

tsteur left a comment

tsteur Nov 11, 2021

tsteur Nov 11, 2021

tsteur commented Nov 15, 2021

fix concurrency archiving overview goals #18243

fix concurrency archiving overview goals #18243

Conversation

peterhashair commented Oct 29, 2021 • edited

Description:

Review

peterhashair commented Oct 31, 2021

tsteur commented Nov 1, 2021

peterhashair commented Nov 3, 2021 • edited

tsteur commented Nov 3, 2021

sgiehl left a comment

Choose a reason for hiding this comment

sgiehl Nov 9, 2021

Choose a reason for hiding this comment

tsteur Nov 10, 2021

Choose a reason for hiding this comment

peterhashair Nov 11, 2021

Choose a reason for hiding this comment

tsteur Nov 11, 2021

Choose a reason for hiding this comment

peterhashair Nov 11, 2021

Choose a reason for hiding this comment

tsteur left a comment

Choose a reason for hiding this comment

tsteur Nov 11, 2021

Choose a reason for hiding this comment

tsteur Nov 11, 2021

Choose a reason for hiding this comment

tsteur commented Nov 15, 2021

peterhashair commented Oct 29, 2021 •

edited

peterhashair commented Nov 3, 2021 •

edited