core:archive Avoid filesystem checks when process was set as finished #16954
Conversation
Because in CliMulti we might check in a loop for every process constantly if they are finished. It won't help all that much but better to not check every time the file system when it is not needed. There be many other things we could improve there. Will add some comments but probably best not to do them (or we'll see if it's needed later)
| @@ -129,6 +134,7 @@ private function pidFileSizeIsNormal() | |||
|
|
|||
| public function finishProcess() | |||
| { | |||
| $this->finished = true; | |||
There was a problem hiding this comment.
technically, once finished, we wouldn't need to call deleteFileIfExists again but we're ignoring this for now just in case for some reason the file was not correctly removed. Could add this check though. finishProcess could be called twice for example during regular job and when climulti:cleanup() is called.
| @@ -81,6 +82,10 @@ public function hasStarted($content = null) | |||
|
|
|||
| public function hasFinished() | |||
| { | |||
| if ($this->finished) { | |||
There was a problem hiding this comment.
technically, I was thinking we should check also if the process hasStarted() and when not has started not declare it as finished. It might be good to eventually do that especially once https://github.com/matomo-org/matomo/pull/16950/files is merged and we then remember if it ever started. The only problem be if a process starts and finishes too quick as part of climulti and then we might not detect the start and therefore don't declare it as finished and then climulti would wait for 8s to declare it as finished but this could happen anyway I suppose https://github.com/matomo-org/matomo/blob/4.0.5/core/CliMulti.php#L217-L222.
I might add this in a separate PR.
There was a problem hiding this comment.
To get around the process quickly starts & finishes issue, we could avoid deleting pid files, and maybe delete them in a task. We'd have to include a timestamp somewhere, though..
I'm not sure how much of a problem it is re archiving though, since those jobs should take a bit to start.
There was a problem hiding this comment.
Yeah I think it's not really an issue and it be rare. Currently, the deletion of the pid file indicates a finished process but we can see. It shouldn't be needed for now I think (I think the method isn't even really called or so unless the process has started etc). I'll see if our issues are fixed with all these changes applied and if not investigate further
mattab
changed the title
Description:
Because in CliMulti we might check in a loop for every process constantly if they are finished. It won't help all that much but better to not check every time the file system when it is not needed.
There be many other things we could improve there. Will add some comments but probably best not to do them (or we'll see if it's needed later)
Technically, we could also set
$this->started=trueand avoid filesystem checks whenstartProcess()is called but shouldn't be needed at least in current code usage. It wouldn't really avoid any file system checks so far and would make it only harder to find some bugs maybe as the filesystem be avoided in the tests potentially.Review