feat(job): add exclusive execution option #2465
Conversation
|
Ok, interesting use of rate-limiter for achieving this effect. A couple of things:
|
|
Another thing to think about, what happens if there are delayed jobs already in the queue, should we care about them? |
src/classes/job.ts
Outdated
| @@ -38,6 +38,7 @@ import type { QueueEvents } from './queue-events'; | |||
| const logger = debuglog('bull'); | |||
|
|
|||
| const optsDecodeMap = { | |||
| ee: 'exclusiveExecution', | |||
There was a problem hiding this comment.
preserveOrder must be used
| const worker = new Worker('queueName', async (job: Job) => { | ||
| // do some work | ||
| }, { | ||
| concurrency: 1 |
There was a problem hiding this comment.
As concurrency is by default 1, I assume it is not required to specify any when using "preserveOrder".
There was a problem hiding this comment.
yes, no necessary, I added it just to be clear, I can add a comment about the default value
|
I think we need more tests for cover the scenario when you have several workers running in parallel. For instance, let's say you have 10 workers (assume all have concurrency 1 AND preserver order flag to true, if any has another set of options then it will not work as expected, we should also write this in the documentation). Now, I assume that as soon one job is fetched, rate limit kicks in and the other workers will just be idling. We need tests to verify that this is the case, also, what happens if the job stalls, it may go back to wait but we need to make sure that no other job waiting is taken by any of the other workers that are idling when this happens, so order is preserved. We also need to document that if you add delayed jobs, then the order cannot be guaranteed either... |
|
It is a lot, but if we want to guarantee order there are several edge cases to consider, probably others we have not thought about yet. |
src/classes/worker.ts
Outdated
| ...this.opts, | ||
| }; | ||
|
|
||
| if (this.opts.stalledInterval <= 0) { | ||
| throw new Error('stalledInterval must be greater than 0'); | ||
| } | ||
|
|
||
| if (this.opts.preserveOrder && this.opts.concurrency > 1) { |
There was a problem hiding this comment.
It would be nice to refactor this check as we are doing exactly the same on "set concurrency". In fact, couldn't we just do this.concurrency = ... here and let the checks from set concurrency take care of it?
There was a problem hiding this comment.
Another thing, shouldn't we have this check on the python version as well?
There was a problem hiding this comment.
I was think on adding it later
src/types/job-options.ts
Outdated
| * If true, it will rate limit the queue when moving this job into delayed. | ||
| * Will stop rate limiting the queue until this job is moved to completed or failed. | ||
| */ | ||
| ee?: boolean; |
There was a problem hiding this comment.
wouldn't this be better as "po" from Preserver Order?
There was a problem hiding this comment.
that option comes from the old version of this pr, I'm going to delete it
there is no global concurrency, so having more than 1 worker will break preserve order behavior, also need to document it. |
No description provided.