[Scheduler] Continue with stored Checkpoint::$time on lock
#52039
Conversation
Checkpoint::$time on lock
601f9f6 to
b649406
Compare
|
I'm trying to understand what you mean here, I'm not deep enough into the topic 😬 Does your description also apply when there is no cache defined? How about this patch instead? --- a/src/Symfony/Component/Scheduler/Generator/Checkpoint.php
+++ b/src/Symfony/Component/Scheduler/Generator/Checkpoint.php
@@ -37,14 +37,12 @@ final class Checkpoint implements CheckpointInterface
return false;
}
- if ($this->reset) {
- $this->reset = false;
- $this->save($now, -1);
- }
-
if ($this->cache) {
[$this->time, $this->index, $this->from] = $this->cache->get($this->name, fn () => [$now, -1, $now]) + [2 => $now];
$this->save($this->time, $this->index);
+ } elseif ($this->reset) {
+ $this->reset = false;
+ $this->save($now, -1);
} |
b649406 to
fe6e951
Compare
That's an interesting case. I think that may indeed be the thought behind this reset. But, if they do have a shared cache, I think worker two can continue where worker one left off. As I'd say that is the purpose of the cache.
With the above reasoning, that patch does indeed work. I've added a test to specifically test a second checkpoint acquiring the lock after the first and test that it continues from the saved time. |
fe6e951 to
fba8bab
Compare
fba8bab to
ab1d7fd
Compare
|
Thank you @Jeroeny. |
When there are multiple message-consumer processes running, the Lock mechanism can be used to ensure a single process takes responsibility of the scheduling.
However when the responsible process releases the lock and a new process acquires it, the state of the checkpoint is reset on purpose.
I'm not sure what the reason behind this is. Maybe I'm missing something and this PR isn't valid. But it seems strange that a Checkpoint stores it's progress which is then lost when that process stops.
Perhaps the 'reset' was more intended like a `reload', since the cached value may have changed by another process? Then this PR would fix it to be an actual reload.
Also I think the change in logic requires this to go to
6.4branch?