[AMQ-8593] Add staticfailover and deprecate masterslave network connector uri

[lucastetreault]

This tweet raised the issue of non-inclusive terminology in the AWS docs and suggested that we should replace masterslave with an more inclusive name for the network connector transport. The AWS docs refer to a feature of ActiveMQ that is a convenience discovery agent: https://activemq.apache.org/networks-of-brokers#masterslave-discovery

Replacing master/slave nomenclature in ActiveMQ was raised in July 2020 AMQ-7514 and there have been some attempts at making some changes (#679, #714, #788) however we have not been able to come to an agreement on nomenclature so these efforts seem to have stalled out.

If we are able to come to an agreement on nomenclature in this PR, we can move forward with removing more non-inclusive terminology on the website (I will follow up with some PRs to the website), in discussions with the community and of course in this codebase. This will remove adoption barriers and make ActiveMQ a more approachable and inclusive project for everyone! Other Apache projects such as Solr and Kafka have moved from master/slave to leader/follower. Leader/follower is also recommended by the IETF and inclusivenaming.org which is supported by companies such as Cisco, Intel, and RedHat.

If we can't come to an agreement on Leader/Follower or some other nomenclature I will, at the very least, create a follow up PR to remove the masterslave transport since it is just a convenience method to use static+failover with ?randomize=false&maxReconnectAttempts=0.

This change leaves the masterslave: transport in place but provides a new alias leaderfollower: for now but we can easily remove it in 5.18.0.

[mattrpav]

I don't think we should just 'drop' masterslave. It will break compatibility for other users.

Can you please create a JIRA to discuss?

I think we should:

1. Add a new discovery agent
2. Mark masterslave as deprecated

[lucastetreault]

@mattrpav this PR actually doesn't remove the masterslave transport: activemq-client/src/main/resources/META-INF/services/org/apache/activemq/transport/discoveryagent/masterslave is updated to use the renamed LeaderFollowerDiscoveryAgentFactory.

I can open a Jira to go along with this PR.

[mattrpav]

@lucastetreault at the very least, it would help to have unit tests showing the old name still works, vs refactoring that test.

.. also the logger gets renamed. I think we should add+deprecate vs surgery.

[lucastetreault]

@mattrpav Jira for this change: https://issues.apache.org/jira/browse/AMQ-8593

[lucastetreault]

@mattrpav I pushed another commit to address your comments. Let me know if there's anything else 👍

[mattrpav]

@lucastetreault thanks for updating to include deprecated for masterslave.

In terms of naming-- I think "ha://" is more accurate (and shorter too!). 'leaderfollower' is not great here, since there is no need to name an architecture. This transport handler really is just 'failover'.

[michaelpearce-gain]

Def needs a deprecation log entry if old config is read up, so that users are alerted maybe if still using old config. A

also im a little -1 using terminology "ha" ha = highly available of which there is many ways to be "highly available" which leader/follower primary/secondary is just one way.... also on naming i believe was front runners for replacement of master/slave terminology. (which this PR originally was doing)

[gemmellr]

I assumed we could merge this in to a 5.17.X release and then it would be fair game to remove in 5.18.0. Is there some written guidance on due notice for this type of change that I could refer to?

5.17.x and 5.18.x wont necessarily be coexisting for as long (or maybe at all) as some previous cases have, and it seems likely some folks may update to one of the earlier 5.17.x releases already existing and stick for a while and not see this in any later 5.17.x releases before e.g trying 5.18.0. Overall, may be better to consider some period of time where the deprecated option availability overlaps the new one rather than just a specific version.

[gemmellr]

Since the docs say this mechanism is essentially an alternative means of specifying static "failover:" transport usages with some unnamed options, and the code seemingly confirms that from looking like just a loop for creating a "failover://(looped-given-servers)?randomize=false&maxReconnectAttempts=0" string, why not just name it more literally about what the config actually does rather than trying to come up with some new abstractive topology name it seems people have a hard time agreeing on.

The other config type is "static":

<networkConnector uri="static:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

The one under discussion differs in using "failover" transports. So what about something like simply "staticfailover":

<networkConnector uri="staticfailover:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

[gemmellr]

Can it use the 'for removal' flag? If so it seems like it should. Though i realise most things arent going to touch the code itself so it would really only be for descriptive purposes.

(I've yet to use it myself so not clear when it came in)

[gemmellr]

Ditto

[gemmellr]

Seems like this should be unwound...but if a change actually needs made, I'd make the licence header a comment rather than Javadoc.

[lucastetreault]

Def needs a deprecation log entry if old config is read up, so that users are alerted maybe if still using old config. A

How about: LOG.warn("masterSlave is deprecated and will be removed in a future release. Use {newname} instead.");

The one under discussion differs in using "failover" transports. So what about something like simply "staticfailover":

I like that. Very literal which is probably good in this case.

I still think it would be valuable to try to get to a consensus on what terms should replace master/slave. Any thoughts on how we can go about that?

[michaelpearce-gain]

@gemmellr

The other config type is "static":

<networkConnector uri="static:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

The one under discussion differs in using "failover" transports. So what about something like simply "staticfailover":

<networkConnector uri="staticfailover:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

Why need to make a new name at all? why is simply a nested of existing not suitable here: e.g.

static:(failover:(uri1,uri2)?randomize=false&maxReconnectAttempts=0)

[michaelpearce-gain]

I still think it would be valuable to try to get to a consensus on what terms should replace master/slave. Any thoughts on how we can go about that?

essentially requires a vote on dev mail list if you need to have a decision, for the community and pmc to cast their votes.

This said, for this change, is it even needed we make a new name.....? Why not go with simply nested of existing names/url transports per same comment to robbie and which i believe you even highlighted in slack yourself? Are we really saving anything from telling users to put :

(already supported)
static:(failover:(uri1,uri2....

vs

staticfailover(uri1,uri2.....

[lucastetreault]

I still think it would be valuable to try to get to a consensus on what terms should replace master/slave. Any thoughts on how we can go about that?

essentially requires a vote on dev mail list if you need to have a decision, for the community and pmc to cast their votes.

This said, for this change, is it even needed we make a new name.....? Why not go with simply nested of existing names/url transports per same comment to robbie and which i believe you even highlighted in slack yourself? Are we really saving anything from telling users to put :

(already supported) static:(failover:(uri1,uri2....

vs

staticfailover(uri1,uri2.....

I'm fine if the solution in the codebase is to simply remove the convenience method. Even so, I think we need to agree on new terms. For example, to update https://activemq.apache.org/networks-of-brokers#masterslave-discovery with new instructions, how do I refer to the pair of brokers using shared storage if not master/slave? I know Matt proposed some wording on slack that dances around the problem, but there are lots of other places in the docs that could use updating as well :)

[michaelpearce-gain]

So to replace wording of master/slave with either primary/secondary or leader/follower as i stated requires a proposal and a vote in dev mailing list plain and simple there is no way to dance around it, in slack or PR's.

But for this PR i think just deprecation (not yet removal) of convenience method is simplest and cleanest. IMO, and unties this PR from that process/decision.

[gemmellr]

@gemmellr

The other config type is "static":

<networkConnector uri="static:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

The one under discussion differs in using "failover" transports. So what about something like simply "staticfailover":

<networkConnector uri="staticfailover:(tcp://host1:61616,tcp://host2:61616,tcp://..)"/>

Why need to make a new name at all? why is simply a nested of existing not suitable here: e.g.

static:(failover:(uri1,uri2)?randomize=false&maxReconnectAttempts=0)

Maybe it would be, though apparently in the past it was considered not to be and this convenience was created instead. At this point I dont see a particular benefit in trying to unwind the convenience, so just changing the prefix seems the simplest to explain to upgraders and the simplest for them to action and so is what I would do. As it actually seemed to have nothing in particular to do with topology or its naming it seemed odd to get caught up on that here at all, and so I suggested an obvious very literal name that fits what its used for.

I'm not against just removing it if everyone else wants to unwind the convenience entirely and then cover things as needed for ungraders, but I thing the rename is simpler for everyone. I dont plan to spend more more time on this than I have already.

[mattrpav]

+1 'staticfailover' makes sense as the uri prefix.

[lucastetreault]

Thanks for all the feedback folks. I have updated the PR to use the staticfailover as proposed by @gemmellr which seems to be right balance of removing non-inclusive language and convenience for users. I plan to email the mailing list for a formal vote on nomenclature as suggested by @michaelpearce-gain on slack 👍

[lucastetreault]

I created a PR for the corresponding doc changes assuming this will be merged and available in 5.17.2 when it is released: apache/activemq-website#83

[lucastetreault]

@jbonofre I see you're merging a ton of PRs and I assume you're preparing for the next release. It would be great to see this included in the next version!

It seems like everyone is happy with this PR at this point. Are there any blockers?

[jbonofre]

@lucastetreault yes, I will take a look on this one. Thanks.

[jbonofre]

Auto closed: it will be superseded by full terminology change for ActiveMQ 7.0.0

[mattrpav]

Re-opening-- This is additive, and can be merged without having to wait for the openwire terminology changes (aka v7.0.0)