-
Notifications
You must be signed in to change notification settings - Fork 0
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
Introduce configuration override #1
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ def run | |
redirect opts | ||
setup_environment opts | ||
set_pool_options opts | ||
set_pool_configuration_override opts | ||
start_pool | ||
end | ||
|
||
|
@@ -38,6 +39,7 @@ def parse_options | |
opt :nosync, "Don't sync logfiles on every write" | ||
opt :pidfile, "PID file location", :type => String, :short => "-p" | ||
opt :environment, "Set RAILS_ENV/RACK_ENV/RESQUE_ENV", :type => String, :short => "-E" | ||
opt :config_manager, "Path to configuration override definition", :type => String, :short => "-C" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sold on the name of this option. Having a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im fine with changing it, not sure --override is the best option. Really, this file is wiring plugins to extension points, of which one exists currently (a config manager). So maybe -p --plugin? Also, this is dependency injection (has some negative connotations in the ruby world, but thats what this is). I wonder if -i --inject would work? Open to other ideas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely not sold on --config-manager, but I like the idea of -p or -i. Needs some more thought, I think... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think I'll wait to see if anyone else has stronger/better opinions. |
||
opt :term_graceful_wait, "On TERM signal, wait for workers to shut down gracefully" | ||
opt :term_graceful, "On TERM signal, shut down workers gracefully" | ||
opt :term_immediate, "On TERM signal, shut down workers immediately (default)" | ||
|
@@ -115,6 +117,19 @@ def set_pool_options(opts) | |
end | ||
end | ||
|
||
def set_pool_configuration_override(opts) | ||
require 'pathname' | ||
if opts[:config_manager] | ||
file = Pathname.new(opts[:config_manager]).expand_path | ||
if file.exist? | ||
require file | ||
Resque::Pool.log "Resque Pool configuration is overriden in #{opts[:config_manager]}" | ||
else | ||
Resque::Pool.log "--config-manager #{file} error: File does not exist" | ||
end | ||
end | ||
end | ||
|
||
def setup_environment(opts) | ||
Resque::Pool.app_name = opts[:appname] if opts[:appname] | ||
ENV["RACK_ENV"] = ENV["RAILS_ENV"] = ENV["RESQUE_ENV"] = opts[:environment] if opts[:environment] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Resque::Pool.config_override = ->(config) { | ||
{ "foo" => 1, "bar" => 2, "baz" => 3, "foo,bar,baz" => 4 } | ||
} |
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 should probably be
refresh_config!
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.
Also noticed that if I new up a Resque::Pool instance and specify a configuration manager, that if I call
pool.config
, I get the original configuration, not the configuration from the configuration manager. I wonder if in specifying a config manager we call refresh_config at that point in time to make sure the config has changed as one might expect. Thoughts?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, I did notice that. We would need to change the
:config
attr reader into a plain method call that refreshed everything. Sound like a reasonable approach?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.
Yep.
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.
Tried implementing it, and it dawned on me. If we do a lot of calls to config in a loop, we will be calling the configuration override a lot. I don't know if that is exactly what we want. Maybe a proxy that only will reload it if it hasn't been loaded in the last five seconds or something?
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.
Lets look/talk about it in person.