-
Notifications
You must be signed in to change notification settings - Fork 8
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 SQS connection pooling #52
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.
Actually looks very good, see minor suggestions above. 🚀 🚀
@@ -1,3 +1,5 @@ | |||
require 'connection_pool' | |||
|
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.
We do not need to require connection_pool
or depend on it - we can provide a NullPool
instead (Prorate gem has a similar setup). Less dependencies that way :-) and if we document connection_pool compatibility people may install it themselves
@@ -24,6 +26,18 @@ def self.submit!(*jobs, **options) | |||
Sqewer::Submitter.default.submit!(*jobs, **options) | |||
end | |||
|
|||
def self.default_connection_pool |
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.
Can we create an accessor pair (client_pool
/ client_pool=
) instead, to adhere by the principle of least surprise? We can then also return a NullPool from the reader if nothing was configured
@@ -21,7 +21,13 @@ def submit!(job, **kwargs_for_send) | |||
else | |||
serializer.serialize(job) | |||
end | |||
connection.send_message(message_body, **kwargs_for_send) | |||
if connection_pool.is_a?(Sqewer::ConnectionMessagebox) |
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.
Can we use a pool always, but with a stand-in NullPool
when none is configured?
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 is a bit trickier, because the message box also uses a connection (it acts as a composed object). Given the API around this, this is the best solution I found. If you have something better in mind, I'd be happy to make it work :)
@@ -28,6 +28,7 @@ | |||
client.delete_queue(queue_url: ENV.fetch('SQS_QUEUE_URL')) rescue Aws::SQS::Errors::NonExistentQueue | |||
|
|||
ENV.delete('SQS_QUEUE_URL') | |||
Sqewer.reset_default_connection_pool! |
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.
If we have an accessor pair we could do an explicit assignment here instead?
Everytime a new
Aws::SQS::Client
is instantiated, requests to retrieve SQS credentials are triggered. Since this happens for merely everySqewer.submit!
and derivates, a lot of SQS credential requests are fired, leading to quite a big slowdown.Here's a small test run with connection pooling:
and without connection pooling:
We basically start an app console, perform some meta-magic on Net::HTTP to print requests that are being made and then fire a few
Sqewer.submit!
. You can observe two GET requests to http://169.254.169.254 happening for every submit where connection pooling is not used.With this PR, there will be a default connection pool available, the same as there was previously a default connection available. The connection pool can also be injected to Sqewer, in a similar way to how a connection would have been previously injected.