Ensure to cancel an archive process if it is already running #13155
Conversation
tsteur
added
the
Pull Request WIP
| @@ -1873,6 +1901,7 @@ private function makeCliMulti() | |||
| $cliMulti->setUrlToPiwik($this->urlToPiwik); | |||
| $cliMulti->setPhpCliConfigurationOptions($this->phpCliConfigurationOptions); | |||
| $cliMulti->setAcceptInvalidSSLCertificate($this->acceptInvalidSSLCertificate); | |||
| $cliMulti->setConcurrentProcessesLimit($this->getConcurrentRequestsPerWebsite()); | |||
There was a problem hiding this comment.
This change was in theory not needed because we only set multiple URLs where it was already set, but ensures we always respect max concurrent requests is being respected.
tsteur
added
Needs Review
| $periodInProgress = $this->isAlreadyArchivingAnyLowerOrThisPeriod($idSite, $period); | ||
| if ($periodInProgress) { | ||
| $this->logger->info("- skipping archiving for period '{period}' because processing the period '{periodcheck}' is already in progress.", array('period' => $period, 'periodcheck' => $periodInProgress)); | ||
| if ($this->isAlreadyArchivingUrl($url, $idSite, $period, $date)) { |
There was a problem hiding this comment.
In theory we wouldn't need this check and could rely on the one in ->before() but figured it might be good to have as when it was just being processed, we do not need to process it later again even when later the job is not running anymore.
|
|
||
| if ($cliMulti->isCommandAlreadyRunning($urlWithSegment)) { | ||
| $this->logger->info("- skipping segment archiving for '{segment}' because such a process is already in progress.", array('segment' => $segment)); | ||
| if ($this->isAlreadyArchivingSegment($urlWithSegment, $idSite, $period, $segment)) { |
There was a problem hiding this comment.
In theory we wouldn't need this check and could rely on the one in ->before() but figured it might be good to have as when it was just being processed, we do not need to process it later again even when later the job is not running anymore.
| $self = $this; | ||
| $request = new Request($url); | ||
| $request->before(function () use ($self, $url, $idSite, $period, $date) { | ||
| if ($self->isAlreadyArchivingUrl($url, $idSite, $period, $date)) { |
There was a problem hiding this comment.
by the time the job is being executed, some seconds, minutes, or hours might have passed and we need to check again if we should execute the job before starting it.
|
The archiving process is hard to reason about in general, but looks good to me. left a comment in slack, just a thought though. |
| if ($self->isAlreadyArchivingSegment($urlWithSegment, $idSite, $period, $segment)) { | ||
| return Request::ABORT; | ||
| } | ||
|
|
||
| $processedSegmentCount++; |
There was a problem hiding this comment.
actually wondering if I should increase $processedSegmentCount before the if even when aborted? @mattab ?
There was a problem hiding this comment.
I'm not sure if it changes anything as it seems only used for logging info on screen. Since it's a very edge case, it does not really matter. Would probably leave it as it is?
|
FYI we ran some tests with it and worked quite nicely. |
…into preventdoublearchiving
…org#13155) * Ensure to cancel an archive process if it is already running * make sure to still launch segments when lower period is already active
In #12702 we added a check to not run the same archiver twice. However, we still noticed the same command appearing multiple times.
After looking through the code for a while I realized that we start max 3 concurrent archivers at a time. And if an option is set, we might even only start 1 archiver at a time.
When archiving any period, we generate the archiving URL for the period plus all segments. If there are 9 segments, there would be 10 URLs. The thing is that we checked at URL generation time whether an URL is already being executed or not. But because we run max 3 jobs concurrent, an archiving job might be only started much later when all previous requests have finished.
This means we should also check at the time just shortly before the execution of an archiver whether such a job is already running.
I have to say I have not tested the script yet but it should work. For now seeing if the tests succeed.