-
Notifications
You must be signed in to change notification settings - Fork 34
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
Make concurrency/0 overridable by workers #16
base: master
Are you sure you want to change the base?
Conversation
96e47b0
to
c92e000
Compare
@sheharyarn rebased 👍 |
Hey @danxexe. Thanks for the PR! I've been thinking about this change and don't see a good reason why |
Sure, my use case actually consists in reducing the concurrency to zero, effectively disabling workers but still allowing new jobs to be scheduled. This is triggered by a deploy routine that waits until all currently running jobs are finished. I understand that there are other possible ways to implement this, but it seems like it would require some architectural changes to |
I thought about this some more, and I think this might be useful. But the problem I see with this is that it can introduce other regressions and potentially break the whole worker queue (if not the entire application) if used incorrectly. Wouldn't it make more sense to do this on the client side where you queue jobs only when you want to? Open to other alternatives that might make sense. |
Yes, like with any extension point, this feature should be used with care. But I can't come up with a broken case except for having the
This can't be done on the client because I want to prevent any pending job from being started for a period of time, that includes jobs that might already be in the queue.
Another alternative I thought about is decoupling the queue from its consumer. In the current implementation, a That was actually my first idea, but since it would be a somewhat big architectural change I opted for the simpler solution of allowing the concurrency value to be overridden and temporarily set to zero. |
The only change introduced in this PR is commit
96e47b0
which allows a worker to overrideconcurrency/0
which makes workers capable of specifying their concurrency dynamically at run-time. This change conflicts with #14 if introduced separately, that's why this branch was based on that PR's branch. This PR can be rebased after #14 is merged.