Allow to configure ActiveMQ Artemis using broker url #24302
Conversation
|
@jbertram Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
spring-projects-issues
added
the
status: waiting-for-triage
|
@jbertram Thank you for signing the Contributor License Agreement! |
|
@jbertram I see some minor code Formatting violations in CI build, for files |
|
@kmandalas the CI build issues have been resolved. |
|
@pivotal-issuemaster @spring-issuemaster can we proceed with the merge? |
philwebb
added
for: team-attention
|
@kmandalas We need to wait until we've created a 2.4.x branch before we can merge this. |
philwebb
removed
the
for: team-attention
There was a problem hiding this comment.
Thanks @jbertram for the proposal.
While I find the idea of deprecating host/port interesting, it doesn't really address the concerns that we've expressed on the original issue. I've added a few comments, can you please have a look?
| params.put(TransportConstants.HOST_PROP_NAME, this.properties.getHost()); | ||
| params.put(TransportConstants.PORT_PROP_NAME, this.properties.getPort()); | ||
| TransportConfiguration transportConfiguration = new TransportConfiguration( | ||
| NettyConnectorFactory.class.getName(), params); |
There was a problem hiding this comment.
How about this transport configuration that we se to NettyConnectionFactory?
@wilkinsona mentioned that on the original issue and we'd like some feedback on that.
There was a problem hiding this comment.
I thought I had already addressed that concern. To be clear, I don't really see what the concern actually is. The URL and all of its parameters are turned into a TransportConfiguration behind the scenes. Everything that can be set directly via a TransportConfiguration can be set with URL parameters. In other words there is no "loss of control" that I can see as @wilkinsona mentioned.
This change makes the Artemis integration essentially equivalent to the ActiveMQ 5.x integration which in my opinion is how it should have been implemented in the first place.
There was a problem hiding this comment.
As far as I can see you did not hence why I am asking here. The code I've referenced makes an explicit setup using NettyConnectorFactory. As far as I can see we'd lose that as soon as an url is set.
There was a problem hiding this comment.
I guess I'm not following. What exactly is the concern with losing the explicit NettyConnectorFactory setup? The only thing which can be set there is host and port. The URL provides the ability to set host and port as well as any other property.
There was a problem hiding this comment.
What exactly is the concern with losing the explicit NettyConnectorFactory setup?
That's most probably the source of the confusion. When a URL is set, no specific transport is set. When a host and port are set a NettyConnectorFactory transport is set. Looking a bit more NettyConnectorFactory seems the default implementation anyway so we'd use that as well.
That wasn't clear hence why we asked explicitly.
There was a problem hiding this comment.
When a URL is set, no specific transport is set.
That's not accurate. Which "transport" is used is determined by the URL's scheme.
There are only 2 ConnectorFactory implementations in Artemis - NettyConnectorFactory and InVMConnectorFactory. To use the NettyConnectorFactory you simply need to use the tcp:// scheme in the URL. To use the InVMConnectorFactory you need to use the vm:// scheme.
There was a problem hiding this comment.
That's not accurate.
Sorry, poor choice of words.
I meant that the auto-configuration doesn't do anything special when a broker url is set while it does something explicit (in code) when a host is set. We're very cautious to not introduce any inconsistency and the reason why I asked you here.
Thanks for the follow-up and the feedback !
...java/org/springframework/boot/autoconfigure/jms/artemis/ArtemisConnectionFactoryFactory.java
Show resolved
Hide resolved
snicoll
added
the
status: waiting-for-feedback
|
I suspect it was flagged with merge amendments as we don't handle deprecated properties this way (don't worry we can take care of it while polishing). |
snicoll
removed
the
status: waiting-for-feedback
|
@jbertram thank you for your contribution. I've polished it to prevent users using the (now) deprecated host property to get their host/port configuration ignored (which is a smell of you having to add an empty broker-url property here). We now rather check if no broker url is set by the user and if a host is set we configure as we previously did. Of course, if a broker url is set, we'll always use that and I've added an extra test to validate that scenario. |
|
@snicoll, sounds great! Thanks for the review and help with polishing. Feel free to @ me on anything in the future related to Artemis that you may have questions about. |
snicoll
changed the title
|
Will do, thanks again for the great support Justin. |
See gh-10739