Skip to content
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 Connection Config class #1110

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Nov 15, 2021

Proposed Changes

Introducing a connection config (name to approve) per created connection out of the ConnectionFactory that is independent of the factory itself. Meaning the config is immutable.

Background on this change:
I had a project in which I had to create two connections to two different servers, so I initially reused the connection factory, but it ended up being the wrong decision, as it is often referred to during the lifetime of the connection.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

I'll update the API once the approach itself is wanted and approved.

@michaelklishin
Copy link
Member

I like this idea but we should recognise that this is definitely a major breaking change that would invalidate a lot of tutorials and blog posts out there. I guess it's fine since we roughly do such changes once a decade 😅

@bollhals
Copy link
Contributor Author

I like this idea but we should recognise that this is definitely a major breaking change that would invalidate a lot of tutorials and blog posts out there. I guess it's fine since we roughly do such changes once a decade 😅

Well it's only the IAuthMechanism that changed and the ConnectionConfig that's new. So I'm not sure how many turtorials would be affected by this.

@michaelklishin
Copy link
Member

michaelklishin commented Dec 1, 2021

Yes, most content on the Web would not use ConnectionConfig which would be confusing at first. That'd be the only reason not to adopt this change.

@bollhals
Copy link
Contributor Author

So should we proceed with this or abandon?

@michaelklishin
Copy link
Member

I personally think the gain isn't significant enough.

@bollhals
Copy link
Contributor Author

One question though, whats the expectation when I change a property on the factory, shall all connextions be picking this change up or not?
(Whatever the expectation is, it should be documented then)

@michaelklishin
Copy link
Member

@bollhals all new connections instantiated by that factory should be picking it up.

@bollhals
Copy link
Contributor Author

@bollhals all new connections instantiated by that factory should be picking it up.

all new means not the old ones?
If so, we'd need a change like this one to do this.

@michaelklishin
Copy link
Member

How can or why would already instantiated connections be affected by a factory change? It has already produced them.

@bollhals
Copy link
Contributor Author

How can or why would already instantiated connections be affected by a factory change? It has already produced them.

E.g. NetworkRecoveryInterval is read once it is used. So it can be changed after the connection was established. Same for TopologyRecovery. Any Property that is accessed inside of the connection/model class is affected.

@lukebakken lukebakken self-requested a review March 8, 2022 17:36
@lukebakken lukebakken added this to the 8.0.0 milestone Mar 8, 2022
@lukebakken lukebakken force-pushed the feature/connectionConfig branch from 6395708 to adc9917 Compare March 8, 2022 17:50
@lukebakken
Copy link
Contributor

I just rebased this on main and will review more thoroughly soon. The idea of immutable configuration seems good to me.

@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022
@lukebakken lukebakken force-pushed the feature/connectionConfig branch 2 times, most recently from 3a4d05d to fd8c750 Compare March 10, 2022 18:59
Move ConnectionConfig class to API, update approval test
@lukebakken lukebakken force-pushed the feature/connectionConfig branch from fd8c750 to 9e91ead Compare March 14, 2022 22:12
@lukebakken lukebakken merged commit d4b49cf into rabbitmq:main Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants