[AMQ-7514] Deprecate configuration that uses non-inclusive language #679

ehossack-aws · 2021-07-08T19:24:10Z

This is the first in a series of changes that are planned to phase out non-inclusive terminology throughout the config, and codebase.

michaelandrepearce · 2021-07-08T19:40:46Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

@@ -152,10 +152,15 @@
    private boolean useShutdownHook = true;
    private boolean useLoggingForShutdownErrors;
    private boolean shutdownOnMasterFailure;
+    private boolean shutdownOnActiveFailure;


Its a private field no need to duplicate just rename the original field. Just need to keep method compatibility

This comment applies generally. No need for me to repeat on every occurance

Hmm good point, my brain didn't catch that as my first pass was assuming spring looked at the actual variable names.
edit: xbean

This was addressed in latest version of PR

michaelandrepearce · 2021-07-08T19:43:43Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

-        if (shutdownOnMasterFailure) {
-            LOG.error("The Master has failed ... shutting down");
+        if (shutdownOnActiveFailure) {
+            LOG.error("The Active has failed ... shutting down");


Active doesnt sound like a good replacement for master
Typically we would refer to master or slave and which one is serving being the current active node.

Active and passive = state

Master / slave was what role the node took.

I think a different word to replace master should be found e.g.(primary/standby) (primary/replica)

The ticket mentions active/standby and I'd tend to agree that it's the clearest (it's also what we use at AWS).

Consider:

of the nodes, only 1 is active serving requests

the other nodes are not pure replicas, as they have shared storage, rather they are on standby over the same FS

It's possible that in the previous replicated leveldb one could think of replica as a good term here, but I think it's slightly disingenuous. (especially if the storage used isn't actually replicated...)

I disagree with the ticket then
Active is state though. Either master or slave can be active. E.g. we talk about a slave being passive and then if master fails it becomes active. We will end up with very confusing terminology.

https://www.theserverside.com/opinion/Master-slave-terminology-alternatives-you-can-use-right-now

Also i would appreciate the discussion is not based on because aws does it. This is an apache project not an aws one.

Quickly reading the ticket actually mentions

New terms:
a. For shared storage: ‘active’ and ’standby’
b. For replication: ‘primary’ and ‘replica'
c. For 'white list' and 'blacklist': 'allow list' and 'deny list'

Like wise earlier in the same ticket it mentions

Here are just a few terms that should be changed: * The following terms are being targeted for change:

'master' and 'slave' should be replaced with the terms 'live' and 'backup''whitelist' and 'blacklist' should be replaced with the terms 'allowlist' and 'denylist'Rename all the git 'master' branches to the term 'main'

A little using terminology within context and avoiding conflicting terminology is needed here.

I agree i think also we historically overloaded the use of master slave too. Especially when leveldb (i think the wording used then was "pure master slave") and replicated kahadb was done. So some legacy nomiculture there with what master slave meant along with active and passive.

So in this case its the replication one, e.g. the broker is either the primary instance or the replica instance .. so primary replica (or primary standby) should maybe be used

Im also keen we align with Artemis broker for consistency sakes on the way forward here.

Happy for others to chip in here. As this is one of those things that once released its a done deal...

I agree with @michaelandrepearce. I've added a comment to AMQ-7514 to push along the discussion.

This group of methods is never read anywhere by the code base. My understanding is that they are legacy hold-over from early replication approach that has been removed long ago. I do not see any reason to deprecate them. Given that we have agreed to make major breaking changes in 5.17.x (remove leveldb), I believe we can safely remove these methods in this pass.

We should not be adding new terms or concepts that do not apply to the code base in this pass.

I did a 'find references' search and yeah, you're right @mattrpav. I think this is also relevant in the discussion about active/passive/standby. It seems reasonable in the scope of this change rather than renaming to actually remove those methods/concepts. Then there is no need to consider the 'existing' terminology.

as per @mattrpav 's AMQ-8316, moved the two concretely deprecated messages out of this PR into #700

michaelandrepearce

Comments inline

mattrpav · 2021-07-09T12:50:50Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

+    /**
+     * @deprecated AMQ-7514 in the move to inclusive nomenclature
+     */
+    @Deprecated


This method is never called in the code base. My understanding is that it was kept to not break configurations from the long-gone active-active replication. I would contend that since we have agreed to remove leveldb from 5.17.x and there are no planned usages for these metods, that this method (and related ones) could safely be removed instead of deprecated.

mattrpav · 2021-07-09T12:54:44Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

            try {
                stop();
            } catch (Exception e) {
-                LOG.error("Failed to stop for master failure", e);
+                LOG.error("Failed to stop for active failure", e);


Updating log messages is tricky, since it can break monitoring in not-so-easy-to-upgrade ways. I think this should be a sub-task conversation on the parent ticket.

How about in the scope of this PR we add both messages, and then the subtask is to remove the non-inclusive messages? That would support a transition period well.

2 messages could yield 2 alerts. I think we break this up into some sub-tasks and add a broker config param to flip b/w the log messages using deprecated terms. I'll update the JIRA with some sub-tasks.

mattrpav · 2021-07-09T12:59:33Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

-        if (shutdownOnMasterFailure) {
-            LOG.error("The Master has failed ... shutting down");
+        if (shutdownOnActiveFailure) {
+            LOG.error("The Active has failed ... shutting down");


This group of methods is never read anywhere by the code base. My understanding is that they are legacy hold-over from early replication approach that has been removed long ago. I do not see any reason to deprecate them. Given that we have agreed to make major breaking changes in 5.17.x (remove leveldb), I believe we can safely remove these methods in this pass.

We should not be adding new terms or concepts that do not apply to the code base in this pass.

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

This removes passiveSlave and waitForSlave and waitForSlaveTimeout which are no longer used

This is the first in a series of changes that will phase out non inclusive terminology throughout the config, and codebase.

ehossack

The base commit #700 and this PR can both be merged without any merge conflicts, pending approval of course.

ehossack · 2022-03-01T18:40:48Z

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java

@@ -152,10 +152,15 @@
    private boolean useShutdownHook = true;
    private boolean useLoggingForShutdownErrors;
    private boolean shutdownOnMasterFailure;
+    private boolean shutdownOnActiveFailure;


This was addressed in latest version of PR

jbonofre · 2023-10-20T05:51:47Z

Auto closed

jbonofre self-requested a review July 8, 2021 19:30

michaelandrepearce reviewed Jul 8, 2021

View reviewed changes

michaelandrepearce requested changes Jul 8, 2021

View reviewed changes

mattrpav reviewed Jul 9, 2021

View reviewed changes

ehossack-aws commented Jul 9, 2021

View reviewed changes

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java Outdated Show resolved Hide resolved

ehossack-aws added 2 commits September 21, 2021 06:24

[AMQ-8316] Remove unused BrokerService methods

cc763a9

This removes passiveSlave and waitForSlave and waitForSlaveTimeout which are no longer used

[AMQ-8390] Deprecate configuration that uses non-inclusive language

e34ae17

This is the first in a series of changes that will phase out non inclusive terminology throughout the config, and codebase.

ehossack-aws force-pushed the inclusive-language-phase-1 branch from a3925c9 to e34ae17 Compare September 21, 2021 15:50

ehossack-aws mentioned this pull request Sep 23, 2021

AMQ-8317 add config param to toggle inclusive terms in logs #714

Closed

ehossack reviewed Mar 1, 2022

View reviewed changes

ehossack mentioned this pull request Mar 1, 2022

[AMQ-8317] Use inclusive terminology in logs and errors #788

Closed

jbonofre closed this Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMQ-7514] Deprecate configuration that uses non-inclusive language #679

[AMQ-7514] Deprecate configuration that uses non-inclusive language #679

ehossack-aws commented Jul 8, 2021

michaelandrepearce Jul 8, 2021

michaelandrepearce Jul 8, 2021

ehossack-aws Jul 8, 2021 •

edited

ehossack Mar 1, 2022

michaelandrepearce Jul 8, 2021 •

edited

ehossack-aws Jul 8, 2021

michaelandrepearce Jul 8, 2021 •

edited

michaelandrepearce Jul 8, 2021 •

edited

jbertram Jul 9, 2021

mattrpav Jul 9, 2021

ehossack-aws Jul 9, 2021

ehossack-aws Aug 20, 2021 •

edited

michaelandrepearce left a comment

mattrpav Jul 9, 2021

mattrpav Jul 9, 2021

ehossack-aws Jul 9, 2021

mattrpav Jul 12, 2021 •

edited

mattrpav Jul 9, 2021

ehossack left a comment

ehossack Mar 1, 2022

jbonofre commented Oct 20, 2023

[AMQ-7514] Deprecate configuration that uses non-inclusive language #679

[AMQ-7514] Deprecate configuration that uses non-inclusive language #679

Conversation

ehossack-aws commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehossack-aws Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelandrepearce Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelandrepearce Jul 8, 2021 • edited

Choose a reason for hiding this comment

michaelandrepearce Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehossack-aws Aug 20, 2021 • edited

Choose a reason for hiding this comment

michaelandrepearce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattrpav Jul 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehossack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Oct 20, 2023

ehossack-aws Jul 8, 2021 •

edited

michaelandrepearce Jul 8, 2021 •

edited

michaelandrepearce Jul 8, 2021 •

edited

michaelandrepearce Jul 8, 2021 •

edited

ehossack-aws Aug 20, 2021 •

edited

mattrpav Jul 12, 2021 •

edited