resque
resque copied to clipboard
Allow users to share a single redis connection across forks within a single worker
Resque will (unless I'm missing something obvious) intentionally disconnect & reconnect its redis connection across forks. This is safe, but results in a lot of connection churn, which can become an issue in environments with a lot of tasks. The redis-rb library exposes a configuration option that allows parent and child processes to share a single redis connection as long as they don't both attempt to use it at the same time. Resque seems to guard against this, so using the option should be safe. This PR enables this by:
- Introducing a
share_redis
configuration option. If truthy, Resque will refrain from explicitly cycling its redis connection after a fork. - Adding that option to the README
- Adding a test.
In local testing in our environment, this change works properly (there aren't any new errors in our jobs), and reduces our connection churn dramatically.
Thanks @isnotajoke! Came across this while also looking for Resque optimizations and it seems like it could be helpful for us as well.
I'm happy to pilot this branch for our jobs but want to make sure I'm not missing something super obvious. @rafaelfranca any thoughts as an expert?
Thanks again. I'll start playing around with this locally today.
+1
Any update on this one @jeremy ? I'm happy to have a go at it unless you feel like responding to the changes requested @isnotajoke
(apologies for the radio silence; haven't had time to take another look at this)
The main reason I opted to not make it the default is that it's a change from previous behavior, and (without in depth knowledge of how Resque's releases are structured), I tend to prefer releasing new behavior as default off initially, changing it to on in subsequent releases if desired (and if there aren't significant bugs or other issues). Happy to defer to convention if we prefer default on here, though.
Good call on the inherit_socket check; will try to make that change this weekend.
Any update on this one? :)
Very interested in this feature as well.
I'd love to have this feature merged. How can I help get it done?
@iloveitaly thanks for the feedback. I'll make some updates based on your suggestions.
While we did briefly test this in our infrastructure, we ultimately addressed the underlying concern (connection churn) at an infrastructure level rather than with this patch. I still think the idea makes sense, but I'd be more confident in this PR if someone could vouch for it working in their environment. (I can help with some basic sanity checking in my environment, but can't commit to a specific date for having that done).