added shared connection option for worker group #266
Conversation
|
@jondot @gabrieljoelc sharing a connection should be OK. Objections to this? better option names? |
lib/sneakers/workergroup.rb
Outdated
| @@ -29,7 +29,9 @@ def run | |||
| worker_classes = worker_classes.call | |||
| end | |||
|
|
|||
| @workers = worker_classes.map{|w| w.new(nil, pool) } | |||
| bunny_connection = config[:share_bunny_connection] ? create_bunny_connection : nil | |||
There was a problem hiding this comment.
Maybe just push this into the private method?
lib/sneakers/workergroup.rb
Outdated
| @@ -52,5 +54,11 @@ def stop | |||
| @stop_flag.set! | |||
| end | |||
|
|
|||
| def create_bunny_connection | |||
|
|
|||
There was a problem hiding this comment.
There's an extra space here and 62.
|
did the requested changes |
|
@gabrieljoelc this generally LGTM. |
| Bunny.new(Sneakers::CONFIG[:amqp], :vhost => Sneakers::CONFIG[:vhost], :heartbeat => Sneakers::CONFIG[:heartbeat], :logger => Sneakers::logger) | ||
| end | ||
|
|
||
| def create_connection_or_nil |
There was a problem hiding this comment.
I'm afraid it's not obvious what the effects of passing in a Bunny::Session instance vs. a nil are. Can you please add a comment explaining that a non-nil connection will be used as is, and a nil will ensure workers open their own?
There was a problem hiding this comment.
@mikebobrov sorry, I know this PR has been open for a while but before we ask @gabrieljoelc to review it one last time, can you please leave a comment as requested above? Thank you.
There was a problem hiding this comment.
sure. no problem. sorry for the delay. i commented out the invocation of the function. hope it's rather helpfull
|
@gabrieljoelc can we get your blessing to merge this? :) |
|
Code looks good but is there a concern that existing applications using sneakers that have a larger number of consumers would have a sudden throughput drop? Here's a good thread discussing this. This answer talks about issues when using a cluster/load balancer and throughput dropping. The poster suggests using a connection pool as an alternative. Should we consider leveraging the |
|
@gabrieljoelc with this PR the default doesn't change: if We have counterexamples where too many connections is a problem: #291. Getting connection pooling right is much harder than it sounds so I'm deeply suspicious of pooling libraries by default. We can consider pooling but it's out of scope for this PR. |
|
|
|
@michaelklishin makes sense. I created #292 as a future alternative.
Also, Sidekiq documentation suggests using
|
|
Should we add something to the README or the wiki? |
|
@gabrieljoelc I added https://github.com/jondot/sneakers/wiki/Configuration#rabbitmq-connections a few days ago. |
No description provided.