-
Notifications
You must be signed in to change notification settings - Fork 51
Support for automatic re-sizing of EC2 pools #86
Conversation
describe AutosizeWorkersJob do | ||
let(:idle_response) { | ||
{ | ||
:workers=>469, |
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.
these should be arithmetic from the actual config to 1) make it clear what your intent is, and 2) make them not break when config changes, because that is surprising
(i.e. config[:maximum_total_workers] - 1
)
thanks for implementing this! |
@robolson Do you approve this PR? |
def self.perform | ||
worker_thresholds = Settings.worker_thresholds | ||
return unless worker_thresholds.present? | ||
redis_connection = Redis.new |
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 works because we use the default Redis settings. Eventually we'll need to pull the connection into a redis constant/global to be shared.
Proposed improvement to |
redis_connection.del(MonitorWorkersJob.REDIS_STATS_KEY) if need_to_upsize || need_to_downsize | ||
if need_to_downsize | ||
workers_to_shutdown = [worker_thresholds[:instance_chunk_size], current_workers-worker_thresholds[:minimum_total_workers]].min | ||
1.upto(workers_to_shutdown) { ShutdownInstanceJob.enqueue_on(worker_thresholds[:autosize_queue], nil) } |
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.
Instead of creating dummy classes we can manually enqueue a class name like this Resque.enqueue_to(worker_thresholds[:autosize_queue], 'ShutdownInstanceJob')
. Is that confusing because you can' tell where ShutdownInstanceJob is coming from?
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.
No, makes perfect sense. Once again I was just copying existing patterns. I'll change.
@robolson How does this look? |
The final settings will probably need tuning as we watch how it works, but this seems like a good start.