Add support for unique jobs #108
Conversation
sheldonreiff
force-pushed
the
unique-jobs
branch
6 times, most recently
from
e1e9611
to
84324fe
Compare
| // remain on the queue. Otherwise, duplicate jobs may be possible. Defaults to +24 hours. | ||
| // See https://book.cakephp.org/4/en/core-libraries/caching.html#configuring-cache-engines. | ||
| 'uniqueCache' => [ | ||
| 'engine' => 'File', |
There was a problem hiding this comment.
Should we suggest a safer default? Using files will not ensure uniqueness across multiple servers.
There was a problem hiding this comment.
Yes, I think that makes sense. I wasn't really considering this example a suggestion, I just went with the simplest example. Maybe this should be called out in the comments above?
There was a problem hiding this comment.
I think we can mention the File engine as only suitable for local development.
There was a problem hiding this comment.
👍 I added a note
src/QueueManager.php
Outdated
| if (!empty($class::$shouldBeUnique)) { | ||
| $uniqueId = static::getUniqueId($class, $method, $data); | ||
|
|
||
| Cache::write($uniqueId, true, 'Cake/Queue.queueUnique'); |
There was a problem hiding this comment.
Should we use the atomic write method add() instead? While it doesn't do anything special for file storage for memcache/redis it offers better atomicity.
There was a problem hiding this comment.
Sure, I'll defer to you on that, I wasn't immediately able to find documentation on add().
There was a problem hiding this comment.
Oh darn, I didn't know we were missing docs for that method. I'll get that fixed.
sheldonreiff
force-pushed
the
unique-jobs
branch
from
84324fe
to
3385918
Compare
| * | ||
| * @var bool | ||
| */ | ||
| public static $shouldBeUnique = false; |
There was a problem hiding this comment.
I'm not sure if this should be in the template or if users should add the property as needed. It doesn't have to be present, absence is assumed false. And really, this applies to maxAttempts as well. Let me know if you have thoughts on this.
There was a problem hiding this comment.
I don't think they need to be in the template. Having them only in the docs is fine. If you wanted to get really fancy, you could add CLI options to the bake task.
There was a problem hiding this comment.
I like the CLI option idea, I threw that in for this. I'll do that for the failed jobs one later and separately.
sheldonreiff
force-pushed
the
unique-jobs
branch
9 times, most recently
from
29212a8
to
e5e35df
Compare
| $this->assertNull(Cache::read($uniqueId, 'Cake/Queue.queueUnique')); | ||
| } | ||
|
|
||
| protected function setupQeueue($extensionArgs = []) |
There was a problem hiding this comment.
| protected function setupQeueue($extensionArgs = []) | |
| protected function setupQueue($extensionArgs = []) |
This typo shows up a few times.
| $uniqueId = static::getUniqueId($class, $method, $data); | ||
|
|
||
| if (Cache::read($uniqueId, 'Cake/Queue.queueUnique')) { | ||
| return; |
There was a problem hiding this comment.
Should this have a debug level log message emitted? It could be useful for debugging why messages that are enqueued are dropped.
There was a problem hiding this comment.
makes sense to me, I added logging if a logger is set
sheldonreiff
force-pushed
the
unique-jobs
branch
from
e5e35df
to
b848b6c
Compare
class-string wasn't being recognized as a valid type to call static methods on
sheldonreiff
force-pushed
the
unique-jobs
branch
from
b848b6c
to
f34aa71
Compare
|
Thank you 🎉 |
This adds support for queueing unique jobs. When enabled for a given job, it will be (nearly) guaranteed that no other jobs with the same data will be present on the queue simultaneously.
Sometimes when the same job with the same payload is queued multiple times before other instances of the job are able to be run, it's not useful to run the job multiple times, once is enough, so this aims to address that.