Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danxexe
Copy link
Contributor

@danxexe danxexe commented Apr 23, 2019

The only change introduced in this PR is commit 96e47b0 which allows a worker to override concurrency/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.

@danxexe danxexe changed the title Feature/overridable concurrency Make concurrency/0 overridable by workers Apr 23, 2019
@danxexe danxexe force-pushed the feature/overridable-concurrency branch from 96e47b0 to c92e000 Compare April 24, 2019 14:28
@danxexe
Copy link
Contributor Author

danxexe commented Apr 24, 2019

@sheharyarn rebased 👍

@sheharyarn
Copy link
Owner

Hey @danxexe. Thanks for the PR!

I've been thinking about this change and don't see a good reason why concurrency should be dynamically set a runtime and not compile-time. Can you explain your use-case a bit so I can understand why would you need to do this at runtime?

@danxexe
Copy link
Contributor Author

danxexe commented May 6, 2019

@sheharyarn

Can you explain your use-case a bit so I can understand why would you need to do this at runtime?

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 que. Allowing workers to override their concurrency value is a simple solution which has other benefits, like allowing the application to temporarily throttle workers or increase throughput without having to introduce complex features to que.

@sheharyarn
Copy link
Owner

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.

@danxexe
Copy link
Contributor Author

danxexe commented Jun 3, 2019

@sheharyarn

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.

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 concurrency/0 function return a value that is not zero or a positive integer. Can you help me understand your worries by giving an example?

Wouldn't it make more sense to do this on the client side where you queue jobs only when you want to?

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.

Open to other alternatives that might make sense.

Another alternative I thought about is decoupling the queue from its consumer. In the current implementation, a Que.Server process is responsible for both keeping the queue state and consuming the queue / starting a job (calling Que.Queue.process). If we split that into two separate processes, one that just keeps the queue state and another the consumes the queue / starts a job, we can achieve the feature of disabling new jobs from being started by temporarily stoping the consumer process. Note that the actual job process would need to be started under a separate supervisor, since you wouldn't want to crash jobs currently being executed when stopping the consumer process.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants