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

Enable custom logic to determine worker configuration #85

Closed
wants to merge 0 commits into from

Conversation

joshuaflanagan
Copy link
Contributor

This change introduces the concept of a configuration loader.

This is intended to replace our team's other PR #84, which Matt closed

To provide your own configuration logic, you set the configuration_loader class variable on Resque::Pool to an object that responds to #call (which can simply be a lambda/Proc).
It also allows more realtime configuration changes by polling the configuration loader each time a job completes. Configuration loaders can optionally implement a #reset! method if they want to re-initialize when the HUP signal is received.

For example, if you wanted to vary the number of worker processes based on a
value stored in Redis, you could do something like:

task resque:pool:setup do
  Resque::Pool.config_loader = lambda {|env|
    worker_count = Redis.current.get("pool_workers_#{env}").to_i
    {"queueA,queueB" => worker_count }
  }
end

Simply changing the value in Redis would allow the pool to dynamically change, without having to send the HUP signal.

This PR will replace the need for changes like #69, #62, and #21, since they can be implemented as custom configuration loaders.

All of the previous configuration logic has been moved to the FileOrHashLoader class. There are a few quirks to the original behavior, so it was simpler to keep it a single source for now. This can likely be simplified if we do not need to keep all of the original behavior. For example, if the ability to load from a Hash is used solely for testing, all of that logic could likely be removed, and tests can just specify a lambda that returns a Hash.

@joshuaflanagan
Copy link
Contributor Author

Rebased on master to merge cleanly

@joshuaflanagan
Copy link
Contributor Author

I see that activity has picked up on resque-pool in the last couple months. We are still hoping to get this PR merged in. Are there any open questions or anything else preventing it from being merged (other than the current merge conflicts, which we can resolve).

@nevans
Copy link
Collaborator

nevans commented May 19, 2015

I would love to merge this PR in, but I haven't had the opportunity to sit down and grok the code or the merge conflicts yet. If you fix up the merge conflicts, that would be super!

@nevans
Copy link
Collaborator

nevans commented May 19, 2015

It looks like the README is inaccurate: "The configuration loader's #call method will be invoked every time a worker completes a job." But I think that the configuration is actually reloaded (unless cached) once per trip through master's main loop. Since the master_sleep only sleeps for up to 1s (unless it has something to do), it will basically reload once per second (regardless of whether or not a job is completed). Or am I missing something there?

@joshuaflanagan
Copy link
Contributor Author

No, you're right - I remember figuring that out later, but never updated
the readme.

On Tue, May 19, 2015 at 9:08 AM, nicholas a. evans <[email protected]

wrote:

It looks like the README is inaccurate: "The configuration loader's #call
method will be invoked every time a worker completes a job." But I think
that the configuration is actually reloaded (unless cached) once per trip
through master's main loop. Since the master_sleep only sleeps for up to 1s
(unless it has something to do), it will basically reload once per second
(regardless of whether or not a job is completed). Or am I missing
something there?


Reply to this email directly or view it on GitHub
#85 (comment).

@joshuaflanagan
Copy link
Contributor Author

I no longer have access to the PeopleAdmin/resque-pool repo (where I originally created this PR). I'm working to get them to update the branch, but don't know how long that might take. I have pushed a rebased/fixed version of this branch to my own repo at https://github.com/joshuaflanagan/resque-pool/tree/config_loader - so you can always merge from there (and then close this PR as accepted). I didn't want to create a new PR, which might just cause more confusion.

@joshuaflanagan
Copy link
Contributor Author

PR was merged in from my other tracking branch f1fd556

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