-
Notifications
You must be signed in to change notification settings - Fork 41.2k
Allow to configure ActiveMQ Artemis using broker url #24302
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aTransportConfiguration
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 moreNettyConnectorFactory
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not accurate. Which "transport" is used is determined by the URL's scheme.
There are only 2
ConnectorFactory
implementations in Artemis -NettyConnectorFactory
andInVMConnectorFactory
. To use theNettyConnectorFactory
you simply need to use thetcp://
scheme in the URL. To use theInVMConnectorFactory
you need to use thevm://
scheme.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.
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 !