-
Notifications
You must be signed in to change notification settings - Fork 523
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
AO3-6895 Reduce the number of badly closed database connections #5041
base: master
Are you sure you want to change the base?
Conversation
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.
Could the extend
happen at initialization instead, so we don't forget to do it for future jobs? It seems not-very-Rails to do it by hand everywhere. Please take a look at the Rubocop comments as well, at least 1 is valid. I think some of the others may be silly, in which case we should suppress via comment rather than just ignore them.
(Linking to https://resque.github.io/docs/HOOKS.html as well because I had a hard time figuring out where that was in the docs)
Rails.application.executor.wrap do | ||
if method.is_a?(Integer) | ||
# TODO: For backwards compatibility, if the "method" is an integer, we | ||
# treat it like an ID and use perform_on_instance instead. But once all | ||
# of the jobs in the queue have been processed (or deleted), we should | ||
# be able to remove this check. | ||
perform_on_instance(method, *args) | ||
else | ||
send(method, *args) | ||
end | ||
end |
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.
Based on this TODO, it sounds like we can delete the if
and just do the following?
Rails.application.executor.wrap do | |
if method.is_a?(Integer) | |
# TODO: For backwards compatibility, if the "method" is an integer, we | |
# treat it like an ID and use perform_on_instance instead. But once all | |
# of the jobs in the queue have been processed (or deleted), we should | |
# be able to remove this check. | |
perform_on_instance(method, *args) | |
else | |
send(method, *args) | |
end | |
end | |
Rails.application.executor.wrap { send(method, *args) } |
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing
)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-6895
Purpose
What does this PR do?
Testing Instructions
This will need a smoke test on staging if test pass.
I think we will only know if it helps when deployed to production, I a can test with tcpdump to see if the connections are being dropped.
References
Are there other relevant issues/pull requests/mailing list discussions?
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
This is inspired by Ben Sheldon and special thanks needs to be given to him in the release notes