-
Notifications
You must be signed in to change notification settings - Fork 22
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
Allow to set connection options on a per server basis #107
Conversation
Verify that we use the correct information when connecting to servers.
@0robustus1 and @JHK I added you as reviewers. Please do on a best effort basis, as I'm aware you're not officially maintainers, but I'd like to get at least also some sanity check from your side. |
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.
Honestly I don't really know that code. However, the mixture of using @client
and options
for different kinds of configuration for a new Bunny instance feels a bit odd. @client
is used to store the per server config already. Also this PR removes the usage of current_host
without removing the method.
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.
It's been years since i last read beetle code. So I'm also not particularly familiar with it.
I think before proceeding we should answer the question why Publisher and Subscriber have a (Never mind misread the code)@server
concept when that is to be controlled by the client.
That's what I would've thought but as far as I understand it, that's not actually true, because there is this code in the publisher and subscriber that does the real thing. In any case I will double check we don't overlook an opportunity to take a more principled route. And as far as I can see, the client is meant to be used for connections to different servers, judging from how the library is used. Only one client is created in processors. |
We've orchestrated the changes with xing-amqp and have setup an end-to-end tests with an isolated environment. We'll now feed back what we've learned and bring this PR to a state where it can be reviewed again. |
Adds a set of tests that verifies that per server connection options are used also for http api requests.
This introduced bigger changes in the queue_properties implementation. We change the way the requests are build in order to be able to use per server settings reliably. Most notably we always derive the api port from the server's port, by prepending a 1. Breaking Change: ================ Configuration.api_port has been removed without replacement. Version has been bumped accordingly.
We will |
This reverts commit 89c84d0.
This adds the possibility to configure connection options on a per server basis.
It introduces a new configuration setting
server_connection_options
which is a Hash, that maps a servername to a hash of options with keyshost, port, user, pass, vhost, ssl
. Those options default to the corresponding settings in the configuration.In essence this allows us to configure specific options for individual servers and leave the rest as is.
This is required to add an AmazonMQ broker, which requires different credentials and TLS.
Breaking changes
This change removes the
Beetle::Configuration.api_port
attribute accessor without a replacement.Instead the API port is always derived from the server's amqp port by prepending a 1.
Implementation note
This change is not the best in terms of software design, but the overall objective for me was to keep the change minimal, and minimize risks of unintended breakage. I don't know the codebase very well, and we intend to update many clients with this change, so we need to play safe.
Additionally, we plan to deprecate and remove the library soon, so it really doesn't make sense to make it extra pretty and ready for evolution.
Release plan
We intent to integrate this change into a new release branch
v4.x
in order to keep upstream relatively undisturbed.This PR targets that branch already.
Testing
We will test this change end-to-end in multiple steps.
Todo
Refs