-
Notifications
You must be signed in to change notification settings - Fork 408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(job): add exclusive execution option #2465
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preserveOrder must be used
const worker = new Worker('queueName', async (job: Job) => { | ||
// do some work | ||
}, { | ||
concurrency: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing, shouldn't we have this check on the python version as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be better as "po" from Preserver Order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
``` | ||
|
||
{% hint style="warning" %} | ||
This feature is only allowed when using concurrency 1, any greater value will throw an error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add another hint, when using retries and backoffs, for instance, a failed job will keep the queue idle during the time the job is being backed off until it is picked again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a couple of comments on the documentation.
@@ -0,0 +1,19 @@ | |||
# Preserve Order | |||
|
|||
BullMQ supports preserving execution order in jobs. When _preserveOrder_ option is provided as *true*, jobs will be processed in the same order, independently of retry strategies. If the current job fails and has a retry strategy, queue will be in rate limit state until the delay is accomplish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> BullMQ supports preserving execution order in jobs. When the preserveOrder option is provided as true, jobs will be processed in the same order, independently of retry strategies. If the current job fails and has a retry strategy, the queue will be in rate limit state until the retry delay has expired.
{% endhint %} | ||
|
||
{% hint style="warning" %} | ||
This feature is only allowed when using concurrency 1, any greater value will throw an error. Make sure to also set a [global concurrency](https://docs.bullmq.io/guide/queues/global-concurrency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> This feature is only allowed when using concurrency 1, any greater value will throw an error. Make sure to also set a global concurrency if you want to preserve order having multiple running workers.
No description provided.