You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@artemis.apache.org by cl...@apache.org on 2026/07/28 04:22:47 UTC
(artemis) 01/03: ARTEMIS-6162 Only start lock coordinators once the broker is activated
This is an automated email from the ASF dual-hosted git repository.
clebertsuconic pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/artemis.git
commit d12d99af276ed5d6f7a9fb4edc24e6500d3ebeab
Author: michael <lu...@gmail.com>
AuthorDate: Tue Jul 21 08:16:57 2026 +0200
ARTEMIS-6162 Only start lock coordinators once the broker is activated
ActiveMQServerImpl.internalStart() started every configured lock
coordinator right away, regardless of the HA role of the node, so a
passive backup contended for the lock along with the active primary.
Two things go wrong with that. A passive backup that wins the lock
holds it while none of its acceptors or broker connections exist yet,
so the elements bound to that coordinator are down on every node of
the group. And when brokers in different data centers share a lock,
the backup of a failed primary needs a few seconds to activate while
the remote brokers keep polling every check-period, so the lock
usually lands on a remote broker rather than on the backup that is
taking over.
Split the creation from the start. The coordinators are still created
in internalStart(), since acceptors and broker connections look them
up by name while they are being created, but polling for the lock now
starts in completeActivation(), the single point every activation
goes through, right before the acceptors are started. Only a node
that actually became active contends for the lock.
Added LockCoordinatorHaActivationTest: a replicated pair sharing one
coordinator. The passive backup keeps its coordinator stopped while
the primary holds the lock, and takes the lock over once the primary
is gone. Without the fix the backup is polling for the lock while
still passive and the test fails.
Assisted-by: Claude
---
.../artemis/core/server/ActiveMQServerLogger.java | 2 +-
.../core/server/impl/ActiveMQServerImpl.java | 29 +++++-
docs/user-manual/lock-coordination.adoc | 2 +
.../failover/LockCoordinatorHaActivationTest.java | 109 +++++++++++++++++++++
4 files changed, 137 insertions(+), 5 deletions(-)
diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java
index 0967cfe46e..4f33690077 100644
--- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java
+++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java
@@ -1525,7 +1525,7 @@ public interface ActiveMQServerLogger {
void lockCoordinatorNotFoundOnAcceptor(String lockName, String acceptorName);
@LogMessage(id = 224156, value = "LockCoordinator {} starting with className={} and lockID={} with checkPeriod={} milliseconds", level = LogMessage.Level.INFO)
- void lockCoordinatorStarting(String lockName, String className, String lockID, int checkPeriod);
+ void lockCoordinatorStarting(String lockName, String className, String lockID, long checkPeriod);
@LogMessage(id = 224157, value = "At least one of the components failed to start under the lockCoordinator {}. A retry will be executed", level = LogMessage.Level.INFO)
void retryLockCoordinator(String name);
diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
index 4fb63b0b00..03248c0511 100644
--- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
+++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
@@ -761,7 +761,7 @@ public class ActiveMQServerImpl implements ActiveMQServer {
ActiveMQServerLogger.LOGGER.serverStarting((haPolicy.isBackup() ? "Backup" : "Primary"), configuration);
- startLockCoordinators();
+ createLockCoordinators();
final boolean wasPrimary = !haPolicy.isBackup();
if (!haPolicy.isBackup()) {
@@ -821,7 +821,15 @@ public class ActiveMQServerImpl implements ActiveMQServer {
}
}
- private void startLockCoordinators() {
+ /**
+ * Instantiates the configured lock managers and creates the {@link LockCoordinator}s, making them available
+ * through {@link #getLockCoordinator(String)} for acceptors and broker connections to look up by name. This runs
+ * unconditionally on every {@link #internalStart()}, well before the server is activated, so the coordinators
+ * must exist here for that lookup to succeed. It does NOT start polling for the lock: that only happens once the
+ * server actually becomes active, in {@link #startLockCoordinators()}, called from {@link #completeActivation}.
+ * Otherwise a passive backup (or any node that hasn't been chosen to activate) would contend for the lock too.
+ */
+ private void createLockCoordinators() {
for (LockCoordinatorConfiguration lockCoordinatorConfiguration : configuration.getLockCoordinatorConfigurations()) {
String className = lockCoordinatorConfiguration.getClassName();
String name = lockCoordinatorConfiguration.getName();
@@ -839,8 +847,20 @@ public class ActiveMQServerImpl implements ActiveMQServer {
LockCoordinator lockCoordinator = new LockCoordinator(scheduledPool, executorFactory.getExecutor(), checkPeriod, lockManager, lockId, name);
lockCoordinators.put(name, lockCoordinator);
- ActiveMQServerLogger.LOGGER.lockCoordinatorStarting(name, className, lockId, checkPeriod);
- lockCoordinator.start();
+ }
+ }
+
+ /**
+ * Starts polling for the distributed lock on every created {@link LockCoordinator} that isn't already started.
+ * Called from {@link #completeActivation} so that only the node that actually became active contends for the
+ * lock.
+ */
+ private void startLockCoordinators() {
+ for (LockCoordinator lockCoordinator : lockCoordinators.values()) {
+ if (!lockCoordinator.isStarted()) {
+ ActiveMQServerLogger.LOGGER.lockCoordinatorStarting(lockCoordinator.getName(), lockCoordinator.getLockManager().getClass().getName(), lockCoordinator.getLockId(), lockCoordinator.getPeriod());
+ lockCoordinator.start();
+ }
}
}
@@ -3733,6 +3753,7 @@ public class ActiveMQServerImpl implements ActiveMQServer {
}
}
}
+ startLockCoordinators();
getRemotingService().startAcceptors();
activationLatch.countDown();
callActivationCompleteCallbacks();
diff --git a/docs/user-manual/lock-coordination.adoc b/docs/user-manual/lock-coordination.adoc
index 92c8679876..76ee841772 100644
--- a/docs/user-manual/lock-coordination.adoc
+++ b/docs/user-manual/lock-coordination.adoc
@@ -135,6 +135,8 @@ The following elements are configured on every lock-coordinator regardless of im
|How often to check if the lock is still valid, in milliseconds
|===
+NOTE: In a journal HA setup with either shared storage or journal replication, lock coordinators will stay inactive and will not poll for the lock until the server is activated.
+
=== File-Based Lock Manager
The file-based lock manager uses the file system to manage distributed locks through file locking mechanisms.
diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/LockCoordinatorHaActivationTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/LockCoordinatorHaActivationTest.java
new file mode 100644
index 0000000000..1e8fa5f756
--- /dev/null
+++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/LockCoordinatorHaActivationTest.java
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.tests.integration.cluster.failover;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.activemq.artemis.api.core.TransportConfiguration;
+import org.apache.activemq.artemis.core.config.Configuration;
+import org.apache.activemq.artemis.core.config.LockCoordinatorConfiguration;
+import org.apache.activemq.artemis.core.server.ActiveMQServer;
+import org.apache.activemq.artemis.core.server.NodeManager;
+import org.apache.activemq.artemis.core.server.impl.InVMNodeManager;
+import org.apache.activemq.artemis.core.server.lock.LockCoordinator;
+import org.apache.activemq.artemis.lockmanager.file.FileBasedLockManager;
+import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
+import org.apache.activemq.artemis.tests.util.ReplicatedBackupUtils;
+import org.apache.activemq.artemis.tests.util.TransportConfigurationUtils;
+import org.apache.activemq.artemis.tests.util.Wait;
+import org.junit.jupiter.api.Test;
+
+/**
+ * A lock coordinator must only start polling for its distributed lock once the broker that owns it is actually
+ * active. Before this was fixed, {@code ActiveMQServerImpl.internalStart()} started every configured lock
+ * coordinator unconditionally, regardless of primary/backup role. As a consequence a passive HA backup (replication
+ * ha-policy) would compete for the lock with the currently active primary and, if it happened to win it, elements
+ * bound to the lock coordinator (acceptors, broker connections) would stay dead on the primary while the passive
+ * backup - which isn't accepting any traffic - silently held the lock.
+ */
+public class LockCoordinatorHaActivationTest extends ActiveMQTestBase {
+
+ private static final String LOCK_NAME = "theLock";
+
+ @Test
+ public void testPassiveBackupDoesNotContendForLock() throws Exception {
+ final String locksFolder = getTemporaryDir();
+
+ final TransportConfiguration primaryConnector = TransportConfigurationUtils.getInVMConnector(true);
+ final TransportConfiguration backupConnector = TransportConfigurationUtils.getInVMConnector(false);
+ final TransportConfiguration backupAcceptor = TransportConfigurationUtils.getInVMAcceptor(false);
+
+ Configuration backupConfig = createDefaultInVMConfig();
+ Configuration primaryConfig = createDefaultInVMConfig();
+
+ ReplicatedBackupUtils.configureReplicationPair(backupConfig, backupConnector, backupAcceptor, primaryConfig, primaryConnector, null);
+ primaryConfig.clearAcceptorConfigurations().addAcceptorConfiguration(TransportConfigurationUtils.getInVMAcceptor(true));
+
+ // both nodes are configured with the very same lock-coordinator, pointing at the very same locks-folder, so
+ // they really do contend for the same file lock if both were to start polling for it.
+ addLockCoordinator(primaryConfig, locksFolder);
+ addLockCoordinator(backupConfig, locksFolder);
+
+ NodeManager primaryNodeManager = new InVMNodeManager(true, primaryConfig.getJournalLocation());
+ ActiveMQServer primaryServer = createInVMFailoverServer(true, primaryConfig, primaryNodeManager, 1);
+
+ NodeManager backupNodeManager = new InVMNodeManager(true, backupConfig.getJournalLocation());
+ ActiveMQServer backupServer = createInVMFailoverServer(true, backupConfig, backupNodeManager, 2);
+
+ primaryServer.start();
+ Wait.assertTrue(primaryServer::isActive);
+ Wait.assertEquals("Locked", () -> primaryServer.getLockCoordinator(LOCK_NAME).getStatus());
+
+ backupServer.start();
+ Wait.assertTrue(backupServer::isReplicaSync, 30_000, 100);
+
+ // the backup is passive: its lock coordinator must have been created (so acceptors/broker connections can
+ // look it up by name) but must stay stopped, never contending for the lock with the active primary.
+ LockCoordinator backupCoordinator = backupServer.getLockCoordinator(LOCK_NAME);
+ assertNotNull(backupCoordinator, "the backup must create its lock coordinators even while passive");
+ assertEquals("Stopped", backupCoordinator.getStatus());
+
+ // give the backup's coordinator a few check-periods worth of time to prove it really isn't polling
+ Thread.sleep(500);
+ assertEquals("Stopped", backupCoordinator.getStatus(), "a passive backup must not contend for the lock");
+ assertEquals("Locked", primaryServer.getLockCoordinator(LOCK_NAME).getStatus(), "the active primary must still be holding the lock");
+
+ // the primary goes away: the backup activates and, only now, is allowed to start polling for (and acquire)
+ // the lock.
+ primaryServer.stop();
+
+ Wait.assertTrue(backupServer::isActive, 30_000, 100);
+ Wait.assertEquals("Locked", () -> backupServer.getLockCoordinator(LOCK_NAME).getStatus(), 30_000, 100);
+ }
+
+ private void addLockCoordinator(Configuration configuration, String locksFolder) {
+ Map<String, String> properties = new HashMap<>();
+ properties.put("locks-folder", locksFolder);
+ LockCoordinatorConfiguration lockCoordinatorConfiguration = new LockCoordinatorConfiguration(properties)
+ .setName(LOCK_NAME).setLockId(LOCK_NAME).setClassName(FileBasedLockManager.class.getName()).setCheckPeriod(100);
+ configuration.addLockCoordinatorConfiguration(lockCoordinatorConfiguration);
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@artemis.apache.org
For additional commands, e-mail: commits-help@artemis.apache.org