Conversation
847191b to
0900aed
Compare
| TopologyMemberImpl member = clusterConnection.getTopology().getMember(server.getNodeID().toString()); | ||
| if (member != null) { | ||
| return member.toBackupURI(); | ||
| TransportConfiguration backupConnector = member.getBackup(); |
There was a problem hiding this comment.
Adding this here seems off if you intend as you've said above for this to be a more generic option applicable to other protocols besides AMQP. Having to copy this code block around for any other implementation that needs it is sub optimal and likely error prone. Seems as though this code should live elsewhere so that it can be called here or in other protocols implementations to check if the connector is to be advertised.
There was a problem hiding this comment.
Currently this fix is specific for AMQP. I could see making this naming AMQP specific if that make it more obvious for now. No matter what there won't be a single spot for all protocols unless an abstraction is specifically made for this but I don't think this is something all protocols support so I wouldn't think that makes sense but let me know if that would stop this getting merged. Core might be only other protocol that I know of that might use this but I could be wrong.
0900aed to
c5021dd
Compare
| @@ -150,6 +150,8 @@ public class TransportConstants { | |||
|
|
|||
| public static final String TRUST_MANAGER_FACTORY_PLUGIN_PROP_NAME = "trustManagerFactoryPlugin"; | |||
There was a problem hiding this comment.
This value should be added to the initialization of ALLOWABLE_CONNECTOR_KEYS from what I can tell the usage is meant to be. Also a test to ensure that the XML parses out the URI options and applies them as this would show that the option is likely not applied currently in that case since it isn't added to the allowed keys set
There was a problem hiding this comment.
Thanks, that makes sense. My manual testing did not seem to need this but I'll add for completeness.
I've added the property to ALLOWABLE_CONNECTOR_KEYS and added a regression test in ConnectorTransportConfigurationParserURITest to verify that clientFailoverAdvertisingEnabled=false is parsed from the connector URI into the TransportConfiguration params.
…t send failover server list info to AMQP clients
c5021dd to
94f9b5e
Compare
|
While this is better than nothing I don't think having just this one option solves issues mentioned in ARTEMIS-6004 in a meaningful way. Topology is not only used for failover transport, and we still need some kind of vehicle to separate what is sent to clients instead which one is used to form a cluster. Ignoring connector endpoints is only part of it. Also, don't forget about dynamic topology discovery which doesn't have static connector configuration to attach this property to. |
@ViliusS Yeah, this seems like it would be a much larger change. My original implementation was primarily intended to address the specific failure scenario described in the ticket, where an internally advertised connector is not reachable by the external client. It sounds like the broader solution is to have a separate client-facing TransportConfiguration from the connector used for internal cluster communication, and have that information carried through topology so it can be consumed by Core and other protocols. That would require changes much deeper in the topology path, including core messages such as NodeAnnounceMessage, rather than just filtering the existing failover connector like I am doing now. If that is the intended direction for ARTEMIS-6004, I think that would be a significantly larger change and likely better handled as a separate MR at this point, with this change serving only as a targeted fix for the specific failure case. If you don't consider the current approach a reasonable interim fix, I'm happy to close this MR. |
Looking purely from the perspective of what I was implying in ARTEMIS-6004, your description is correct. I was thinking the same way, about some kind of separate "topology configuration" option which would define full topology and would cover other issues described in the ticket. As for this pull request, I'm not against it. I'm not on Artemis team, and I don't have enough knowledge about Artemis codebase, so in the end it's up to them to decide if this interim fix is reasonable. I just wanted to clearly indicate that this doesn't cover the ticket fully. Maybe the ticket can be split per use case? I'm not sure what is the best approach here. |
|
I'll wait to see if I can get some feedback on this MR if possible. I'll have limited access for the next week. There would need to be more discussion on a design before tackling another approach since it's very open ended right now. I may be able to look into an initial design and refine on Jira if I'm told this is not good and needs to be closed, but would need to get specific requirements. I do think that this MR does resolve a problem that I have seen and I wanted to provide a solution in the meantime. |
3 participants
Description
Add a
clientFailoverAdvertisingEnabledconnector property to control whether a backup connector is advertised to clients as part of the AMQP failover topology.Currently, Artemis can advertise the backup connector from the broker's internal cluster topology to AMQP clients. This can cause clients using different internal and external DNS names or ports to receive an unreachable failover endpoint.
For example, an external Qpid JMS client may initially connect using:
but receive the internal backup connector from the broker topology:
The client can then attempt to use the advertised endpoint instead of the endpoint configured by the client.
This is the scenario described in ARTEMIS-6004.
Changes
clientFailoverAdvertisingEnabledconnector property.trueto preserve existing behavior.false, the backup connector is not included in the AMQP failover information advertised to clients.Testing
The change was tested with a Java Qpid JMS client using an Artemis HA live/backup configuration where the broker's internal cluster topology uses different hostnames/ports from those exposed to external clients.
With:
the broker no longer advertises the internal backup connector to the AMQP client.
As a result, external clients no longer need to configure:
to prevent Qpid JMS from replacing their configured failover endpoints with the broker-provided topology.
This provides a broker-side solution for clients that cannot be configured with
failover.amqpOpenServerListAction=IGNORE.