@peterhashair opened this Pull Request on October 29th 2021 Contributor

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 commented on October 31st 2021 Contributor

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

@tsteur commented on November 1st 2021 Member

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

@peterhashair commented on November 3rd 2021 Contributor

@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?
https://github.com/matomo-org/matomo/blob/0475bc2717c52fb864739b64328c36aa508f6a4e/core/ArchiveProcessor/Loader.php#L136-L171

@tsteur commented on November 3rd 2021 Member

@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 https://github.com/matomo-org/matomo/pull/18255 is merged

@tsteur commented on November 15th 2021 Member

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

This Pull Request was closed on November 15th 2021
Powered by GitHub Issue Mirror