From 56c10f1bcaec6dc1e9eda95eed7052a1938ed3ef Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Wed, 3 Sep 2025 22:49:35 +0000 Subject: [PATCH 1/5] Removes the need for the LockWatcherWrapper Removes the LockWatcher interface and switches to using the AccumuloLockWatcher. Removes the need for a LockWatcherWrapper Adds a boolean to track lockAcquired state. Made boolean outside of watcher to follow original LockWatcherWrapper implementation. --- .../accumulo/core/lock/ServiceLock.java | 59 +++++-------------- .../core/lock/ServiceLockSupport.java | 26 +++++++- .../MiniAccumuloClusterImpl.java | 5 ++ .../apache/accumulo/server/util/Admin.java | 10 ++++ .../apache/accumulo/compactor/Compactor.java | 7 ++- .../apache/accumulo/tserver/ScanServer.java | 7 ++- .../apache/accumulo/tserver/TabletServer.java | 7 ++- .../apache/accumulo/test/fate/TestLock.java | 12 +++- .../test/functional/ZombieTServer.java | 22 ++++++- .../accumulo/test/lock/ServiceLockIT.java | 10 ++++ .../test/performance/NullTserver.java | 9 +++ 11 files changed, 115 insertions(+), 59 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java index 9494e419798..3c964380589 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java @@ -70,26 +70,28 @@ public enum LockLossReason { LOCK_DELETED, SESSION_EXPIRED } - public interface LockWatcher { + public interface AccumuloLockWatcher { + + boolean isLocked(); + + void acquiredLock(); + + void failedToAcquireLock(Exception e); + void lostLock(LockLossReason reason); /** * lost the ability to monitor the lock node, and its status is unknown */ void unableToMonitorLockNode(Exception e); - } - public interface AccumuloLockWatcher extends LockWatcher { - void acquiredLock(); - - void failedToAcquireLock(Exception e); } private final ServiceLockPath path; protected final ZooSession zooKeeper; private final Prefix vmLockPrefix; - private LockWatcher lockWatcher; + private AccumuloLockWatcher lockWatcher; private String lockNodeName; private volatile boolean lockWasAcquired; private volatile boolean watchingParent; @@ -110,45 +112,12 @@ public ServiceLock(ZooSession zookeeper, ServiceLockPath path, UUID uuid) { } } - private static class LockWatcherWrapper implements AccumuloLockWatcher { - - boolean acquiredLock = false; - final LockWatcher lw; - - public LockWatcherWrapper(LockWatcher lw2) { - this.lw = lw2; - } - - @Override - public void acquiredLock() { - acquiredLock = true; - } - - @Override - public void failedToAcquireLock(Exception e) { - LOG.debug("Failed to acquire lock", e); - } - - @Override - public void lostLock(LockLossReason reason) { - lw.lostLock(reason); - } - - @Override - public void unableToMonitorLockNode(Exception e) { - lw.unableToMonitorLockNode(e); - } - - } - - public synchronized boolean tryLock(LockWatcher lw, ServiceLockData lockData) + public synchronized boolean tryLock(AccumuloLockWatcher lw, ServiceLockData lockData) throws KeeperException, InterruptedException { - LockWatcherWrapper lww = new LockWatcherWrapper(lw); - - lock(lww, lockData); + lock(lw, lockData); - if (lww.acquiredLock) { + if (lw.isLocked()) { return true; } @@ -368,7 +337,7 @@ public void process(WatchedEvent event) { } private void lostLock(LockLossReason reason) { - LockWatcher localLw = lockWatcher; + AccumuloLockWatcher localLw = lockWatcher; lockNodeName = null; lockId = null; lockWatcher = null; @@ -533,7 +502,7 @@ public synchronized void unlock() throws InterruptedException, KeeperException { throw new IllegalStateException(); } - LockWatcher localLw = lockWatcher; + AccumuloLockWatcher localLw = lockWatcher; String localLock = lockNodeName; lockNodeName = null; diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java index fad535fefa5..58e7dfa3021 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java @@ -18,6 +18,7 @@ */ package org.apache.accumulo.core.lock; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.function.Supplier; @@ -26,7 +27,6 @@ import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy; import org.apache.accumulo.core.lock.ServiceLock.AccumuloLockWatcher; import org.apache.accumulo.core.lock.ServiceLock.LockLossReason; -import org.apache.accumulo.core.lock.ServiceLock.LockWatcher; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import org.apache.accumulo.core.util.Halt; import org.apache.zookeeper.KeeperException; @@ -103,6 +103,11 @@ public synchronized void acquiredLock() { notifyAll(); } + @Override + public boolean isLocked() { + return acquiredLock; + } + @Override public synchronized void failedToAcquireLock(Exception e) { LOG.warn("Failed to get {} lock", server, e); @@ -147,8 +152,10 @@ public boolean isFailedToAcquireLock() { /** * Lock Watcher used by non-HA services */ - public static class ServiceLockWatcher implements LockWatcher { + public static class ServiceLockWatcher implements AccumuloLockWatcher { + private final AtomicBoolean lockAcquired = new AtomicBoolean(false); + private final Logger log = LoggerFactory.getLogger(ServiceLock.class); private final Type server; private final Supplier shutdownComplete; private final Consumer lostLockAction; @@ -160,6 +167,21 @@ public ServiceLockWatcher(Type server, Supplier shutdownComplete, this.lostLockAction = lostLockAction; } + @Override + public boolean isLocked() { + return lockAcquired.get(); + } + + @Override + public void acquiredLock() { + lockAcquired.getAndSet(true); + } + + @Override + public void failedToAcquireLock(Exception e) { + log.debug("Failed to acquire lock", e); + } + @Override public void lostLock(final LockLossReason reason) { if (shutdownComplete.get()) { diff --git a/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java b/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java index 63aa27227df..800a47aa81b 100644 --- a/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java +++ b/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java @@ -673,6 +673,11 @@ public void unableToMonitorLockNode(Exception e) { miniLock = null; } + @Override + public boolean isLocked() { + return lockAcquired.get(); + } + @Override public void acquiredLock() { log.debug("Acquired ZK lock for MiniAccumuloClusterImpl"); diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java index b89c3e2069f..78c31911e26 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java @@ -41,6 +41,7 @@ import java.util.TreeSet; import java.util.UUID; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -365,6 +366,8 @@ static class FateOpsCommand extends SubCommandOpts { List instanceTypes = new ArrayList<>(); } + AtomicBoolean lockAcquired = new AtomicBoolean(false); + class AdminLockWatcher implements ServiceLock.AccumuloLockWatcher { @Override public void lostLock(ServiceLock.LockLossReason reason) { @@ -387,11 +390,18 @@ public void unableToMonitorLockNode(Exception e) { public void acquiredLock() { lockAcquiredLatch.countDown(); log.debug("Acquired ZooKeeper lock for Admin"); + lockAcquired.getAndSet(true); + } + + @Override + public boolean isLocked() { + return lockAcquired.get(); } @Override public void failedToAcquireLock(Exception e) { log.warn("Failed to acquire ZooKeeper lock for Admin, msg: " + e.getMessage()); + lockAcquired.getAndSet(false); } } diff --git a/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java b/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java index 9772d4bd9f6..503be0f56d1 100644 --- a/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java +++ b/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java @@ -83,7 +83,7 @@ import org.apache.accumulo.core.file.FileSKVIterator; import org.apache.accumulo.core.iteratorsImpl.system.SystemIteratorUtil; import org.apache.accumulo.core.lock.ServiceLock; -import org.apache.accumulo.core.lock.ServiceLock.LockWatcher; +import org.apache.accumulo.core.lock.ServiceLock.AccumuloLockWatcher; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptor; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptors; @@ -373,8 +373,9 @@ protected void announceExistence(HostAndPort clientAddress) getContext().getServerPaths().createCompactorPath(getResourceGroup(), clientAddress); ServiceLockSupport.createNonHaServiceLockPath(Type.COMPACTOR, zoo, path); compactorLock = new ServiceLock(getContext().getZooSession(), path, compactorId); - LockWatcher lw = new ServiceLockWatcher(Type.COMPACTOR, () -> getShutdownComplete().get(), - (type) -> getContext().getLowMemoryDetector().logGCInfo(getConfiguration())); + AccumuloLockWatcher lw = + new ServiceLockWatcher(Type.COMPACTOR, () -> getShutdownComplete().get(), + (type) -> getContext().getLowMemoryDetector().logGCInfo(getConfiguration())); try { for (int i = 0; i < 25; i++) { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index f218f8f71f4..87f7c162073 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -70,7 +70,7 @@ import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy; import org.apache.accumulo.core.file.blockfile.cache.impl.BlockCacheConfiguration; import org.apache.accumulo.core.lock.ServiceLock; -import org.apache.accumulo.core.lock.ServiceLock.LockWatcher; +import org.apache.accumulo.core.lock.ServiceLock.AccumuloLockWatcher; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptor; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptors; @@ -320,8 +320,9 @@ private ServiceLock announceExistence() { ServiceLockSupport.createNonHaServiceLockPath(Type.SCAN_SERVER, zoo, zLockPath); serverLockUUID = UUID.randomUUID(); scanServerLock = new ServiceLock(getContext().getZooSession(), zLockPath, serverLockUUID); - LockWatcher lw = new ServiceLockWatcher(Type.SCAN_SERVER, () -> getShutdownComplete().get(), - (type) -> context.getLowMemoryDetector().logGCInfo(getConfiguration())); + AccumuloLockWatcher lw = + new ServiceLockWatcher(Type.SCAN_SERVER, () -> getShutdownComplete().get(), + (type) -> context.getLowMemoryDetector().logGCInfo(getConfiguration())); for (int i = 0; i < 120 / 5; i++) { zoo.putPersistentData(zLockPath.toString(), new byte[0], NodeExistsPolicy.SKIP); diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java index 1861389febc..768953f5a4f 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java @@ -79,7 +79,7 @@ import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy; import org.apache.accumulo.core.file.blockfile.cache.impl.BlockCacheConfiguration; import org.apache.accumulo.core.lock.ServiceLock; -import org.apache.accumulo.core.lock.ServiceLock.LockWatcher; +import org.apache.accumulo.core.lock.ServiceLock.AccumuloLockWatcher; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptor; import org.apache.accumulo.core.lock.ServiceLockData.ServiceDescriptors; @@ -488,8 +488,9 @@ private void announceExistence() { UUID tabletServerUUID = UUID.randomUUID(); tabletServerLock = new ServiceLock(getContext().getZooSession(), zLockPath, tabletServerUUID); - LockWatcher lw = new ServiceLockWatcher(Type.TABLET_SERVER, () -> getShutdownComplete().get(), - (type) -> context.getLowMemoryDetector().logGCInfo(getConfiguration())); + AccumuloLockWatcher lw = + new ServiceLockWatcher(Type.TABLET_SERVER, () -> getShutdownComplete().get(), + (type) -> context.getLowMemoryDetector().logGCInfo(getConfiguration())); for (int i = 0; i < 120 / 5; i++) { zoo.putPersistentData(zLockPath.toString(), new byte[0], NodeExistsPolicy.SKIP); diff --git a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java index 598aed5c7e7..3538955a511 100644 --- a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java +++ b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java @@ -20,6 +20,7 @@ import java.util.UUID; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.data.ResourceGroupId; import org.apache.accumulo.core.fate.user.UserFateStore; @@ -88,6 +89,8 @@ public ServiceLock createTestLock(ServerContext context) throws InterruptedExcep return lock; } + private final AtomicBoolean lockAcquired = new AtomicBoolean(false); + class TestLockWatcher implements ServiceLock.AccumuloLockWatcher { @Override @@ -104,11 +107,18 @@ public void unableToMonitorLockNode(Exception e) { public void acquiredLock() { lockAcquiredLatch.countDown(); log.debug("Acquired ZooKeeper lock for test"); + lockAcquired.getAndSet(true); + } + + @Override + public boolean isLocked() { + return lockAcquired.get(); } @Override public void failedToAcquireLock(Exception e) { - log.warn("Failed to acquire ZooKeeper lock for test, msg: " + e.getMessage()); + log.warn("Failed to acquire ZooKeeper lock for test", e); + lockAcquired.getAndSet(false); } } } diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java index 2e9768397a9..e50d45b8d62 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java @@ -23,6 +23,7 @@ import java.util.HashMap; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.cli.ConfigOpts; import org.apache.accumulo.core.clientImpl.thrift.ClientService; @@ -33,8 +34,8 @@ import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter; import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy; import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.lock.ServiceLock.AccumuloLockWatcher; import org.apache.accumulo.core.lock.ServiceLock.LockLossReason; -import org.apache.accumulo.core.lock.ServiceLock.LockWatcher; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockData.ThriftService; import org.apache.accumulo.core.manager.thrift.TabletServerStatus; @@ -144,7 +145,24 @@ public static void main(String[] args) throws Exception { metricsInfo.init(MetricsInfo.serviceTags(context.getInstanceName(), "zombie.server", serverPort.address, ResourceGroupId.DEFAULT)); - LockWatcher lw = new LockWatcher() { + final AtomicBoolean acquiredLock = new AtomicBoolean(false); + + AccumuloLockWatcher lw = new AccumuloLockWatcher() { + + @Override + public boolean isLocked() { + return acquiredLock.get(); + } + + @Override + public void acquiredLock() { + + } + + @Override + public void failedToAcquireLock(Exception e) { + log.warn("Failed to acquire lock", e); + } @SuppressFBWarnings(value = "DM_EXIT", justification = "System.exit() is a bad idea here, but okay for now, since it's a test") diff --git a/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java b/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java index 22f9ec72f14..ef9838951c1 100644 --- a/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java +++ b/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java @@ -127,6 +127,11 @@ public void lostLock(LockLossReason reason) { @Override public void unableToMonitorLockNode(final Exception e) {} + @Override + public boolean isLocked() { + return lockHeld; + } + @Override public void acquiredLock() { this.lockHeld = true; @@ -156,6 +161,11 @@ public synchronized void lostLock(LockLossReason reason) { this.notifyAll(); } + @Override + public boolean isLocked() { + return locked; + } + @Override public synchronized void acquiredLock() { this.locked = true; diff --git a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java index b03abdb8a8e..4fb2437847e 100644 --- a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java +++ b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.cli.ConfigOpts; import org.apache.accumulo.core.cli.Help; @@ -322,6 +323,7 @@ public static void main(String[] args) throws Exception { HostAndPort.fromParts(ConfigOpts.BIND_ALL_ADDRESSES, opts.port)); sa.startThriftServer("null tserver"); + AtomicBoolean acquiredLock = new AtomicBoolean(false); AccumuloLockWatcher miniLockWatcher = new AccumuloLockWatcher() { @Override @@ -334,14 +336,21 @@ public void unableToMonitorLockNode(Exception e) { LOG.warn("Unable to monitor lock: " + e.getMessage()); } + @Override + public boolean isLocked() { + return acquiredLock.get(); + } + @Override public void acquiredLock() { LOG.debug("Acquired ZooKeeper lock for NullTserver"); + acquiredLock.getAndSet(true); } @Override public void failedToAcquireLock(Exception e) { LOG.warn("Failed to acquire ZK lock for NullTserver, msg: " + e.getMessage()); + acquiredLock.getAndSet(false); } }; From 9d6575ab536064c7503139cb239d10fdf96e78c2 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Sat, 27 Sep 2025 03:48:19 +0000 Subject: [PATCH 2/5] Address PR feedback and renamed inverted method Switched lockAcquired to false if failed to acquire lock. Renamed isFailedToAquireLock to cannotRetryLocking. --- .../org/apache/accumulo/core/lock/ServiceLockSupport.java | 5 +++-- .../java/org/apache/accumulo/gc/SimpleGarbageCollector.java | 2 +- .../src/main/java/org/apache/accumulo/manager/Manager.java | 2 +- .../src/main/java/org/apache/accumulo/monitor/Monitor.java | 4 ++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java index 58e7dfa3021..20b717d3a07 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java @@ -143,8 +143,8 @@ public boolean isLockAcquired() { return acquiredLock; } - public boolean isFailedToAcquireLock() { - return failedToAcquireLock; + public boolean cannotRetryLocking() { + return acquiredLock && !failedToAcquireLock; } } @@ -180,6 +180,7 @@ public void acquiredLock() { @Override public void failedToAcquireLock(Exception e) { log.debug("Failed to acquire lock", e); + lockAcquired.getAndSet(false); } @Override diff --git a/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java b/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java index bc5edfa1681..42bcac77b3a 100644 --- a/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java +++ b/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java @@ -405,7 +405,7 @@ private void getZooLock(HostAndPort addr) throws KeeperException, InterruptedExc break; } - if (!gcLockWatcher.isFailedToAcquireLock()) { + if (gcLockWatcher.cannotRetryLocking()) { throw new IllegalStateException("gc lock in unknown state"); } diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java index 3eb2d5437f9..8d684cb2dff 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java @@ -1401,7 +1401,7 @@ private ServiceLockData getManagerLock(final ServiceLockPath zManagerLoc) break; } - if (!managerLockWatcher.isFailedToAcquireLock()) { + if (managerLockWatcher.cannotRetryLocking()) { throw new IllegalStateException("manager lock in unknown state"); } diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java index 070b93266d5..94425cf7d51 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java @@ -760,7 +760,7 @@ private void getMonitorLock(HostAndPort monitorLocation) UUID zooLockUUID = UUID.randomUUID(); monitorLock = new ServiceLock(context.getZooSession(), monitorLockPath, zooLockUUID); HAServiceLockWatcher monitorLockWatcher = - new HAServiceLockWatcher(Type.MONITOR, () -> isShutdownRequested()); + new HAServiceLockWatcher(Type.MONITOR, this::isShutdownRequested); while (true) { monitorLock.lock(monitorLockWatcher, @@ -774,7 +774,7 @@ private void getMonitorLock(HostAndPort monitorLocation) break; } - if (!monitorLockWatcher.isFailedToAcquireLock()) { + if (monitorLockWatcher.cannotRetryLocking()) { throw new IllegalStateException("monitor lock in unknown state"); } From 075c4637e7bb2062571ef4b8527e8edc305cb9a8 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Mon, 29 Sep 2025 17:30:32 +0000 Subject: [PATCH 3/5] removed isLocked method --- .../org/apache/accumulo/core/lock/ServiceLock.java | 5 +---- .../apache/accumulo/core/lock/ServiceLockSupport.java | 10 ---------- .../miniclusterImpl/MiniAccumuloClusterImpl.java | 5 ----- .../java/org/apache/accumulo/server/util/Admin.java | 5 ----- .../java/org/apache/accumulo/test/fate/TestLock.java | 5 ----- .../apache/accumulo/test/functional/ZombieTServer.java | 5 ----- .../org/apache/accumulo/test/lock/ServiceLockIT.java | 10 ---------- .../apache/accumulo/test/performance/NullTserver.java | 5 ----- 8 files changed, 1 insertion(+), 49 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java index d075d08033b..85260a4c13c 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java @@ -74,8 +74,6 @@ public enum LockLossReason { public interface AccumuloLockWatcher { - boolean isLocked(); - void acquiredLock(); void failedToAcquireLock(Exception e); @@ -118,8 +116,7 @@ public synchronized boolean tryLock(AccumuloLockWatcher lw, ServiceLockData lock throws KeeperException, InterruptedException { lock(lw, lockData); - - if (lw.isLocked()) { + if (lockWasAcquired) { return true; } diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java index 20b717d3a07..f70e4936608 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java @@ -103,11 +103,6 @@ public synchronized void acquiredLock() { notifyAll(); } - @Override - public boolean isLocked() { - return acquiredLock; - } - @Override public synchronized void failedToAcquireLock(Exception e) { LOG.warn("Failed to get {} lock", server, e); @@ -167,11 +162,6 @@ public ServiceLockWatcher(Type server, Supplier shutdownComplete, this.lostLockAction = lostLockAction; } - @Override - public boolean isLocked() { - return lockAcquired.get(); - } - @Override public void acquiredLock() { lockAcquired.getAndSet(true); diff --git a/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java b/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java index d2cd35bc8ab..91c019134ca 100644 --- a/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java +++ b/minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java @@ -681,11 +681,6 @@ public void unableToMonitorLockNode(Exception e) { miniLock = null; } - @Override - public boolean isLocked() { - return lockAcquired.get(); - } - @Override public void acquiredLock() { log.debug("Acquired ZK lock for MiniAccumuloClusterImpl"); diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java index 22001705d7b..cd9594d4d03 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java @@ -403,11 +403,6 @@ public void acquiredLock() { lockAcquired.getAndSet(true); } - @Override - public boolean isLocked() { - return lockAcquired.get(); - } - @Override public void failedToAcquireLock(Exception e) { log.warn("Failed to acquire ZooKeeper lock for Admin, msg: " + e.getMessage()); diff --git a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java index 3538955a511..8a730211f2f 100644 --- a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java +++ b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java @@ -110,11 +110,6 @@ public void acquiredLock() { lockAcquired.getAndSet(true); } - @Override - public boolean isLocked() { - return lockAcquired.get(); - } - @Override public void failedToAcquireLock(Exception e) { log.warn("Failed to acquire ZooKeeper lock for test", e); diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java index ae0b7cd343a..ea23a38a0c4 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java @@ -149,11 +149,6 @@ public static void main(String[] args) throws Exception { AccumuloLockWatcher lw = new AccumuloLockWatcher() { - @Override - public boolean isLocked() { - return acquiredLock.get(); - } - @Override public void acquiredLock() { diff --git a/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java b/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java index ef9838951c1..22f9ec72f14 100644 --- a/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java +++ b/test/src/main/java/org/apache/accumulo/test/lock/ServiceLockIT.java @@ -127,11 +127,6 @@ public void lostLock(LockLossReason reason) { @Override public void unableToMonitorLockNode(final Exception e) {} - @Override - public boolean isLocked() { - return lockHeld; - } - @Override public void acquiredLock() { this.lockHeld = true; @@ -161,11 +156,6 @@ public synchronized void lostLock(LockLossReason reason) { this.notifyAll(); } - @Override - public boolean isLocked() { - return locked; - } - @Override public synchronized void acquiredLock() { this.locked = true; diff --git a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java index 745dfb0405e..f60aaea14de 100644 --- a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java +++ b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java @@ -336,11 +336,6 @@ public void unableToMonitorLockNode(Exception e) { LOG.warn("Unable to monitor lock: " + e.getMessage()); } - @Override - public boolean isLocked() { - return acquiredLock.get(); - } - @Override public void acquiredLock() { LOG.debug("Acquired ZooKeeper lock for NullTserver"); From f2e7203d24793c64956a2ed0932c2a899c3dd1a8 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Mon, 29 Sep 2025 17:42:05 +0000 Subject: [PATCH 4/5] Remove extra booleans and use isLocked --- .../main/java/org/apache/accumulo/core/lock/ServiceLock.java | 2 +- .../src/main/java/org/apache/accumulo/server/util/Admin.java | 5 ----- .../main/java/org/apache/accumulo/test/fate/TestLock.java | 5 ----- .../org/apache/accumulo/test/functional/ZombieTServer.java | 3 --- .../org/apache/accumulo/test/performance/NullTserver.java | 4 ---- 5 files changed, 1 insertion(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java index 85260a4c13c..01941239259 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java @@ -116,7 +116,7 @@ public synchronized boolean tryLock(AccumuloLockWatcher lw, ServiceLockData lock throws KeeperException, InterruptedException { lock(lw, lockData); - if (lockWasAcquired) { + if (isLocked()) { return true; } diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java index cd9594d4d03..c1b66673661 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java @@ -41,7 +41,6 @@ import java.util.TreeSet; import java.util.UUID; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -376,8 +375,6 @@ static class FateOpsCommand extends SubCommandOpts { List instanceTypes = new ArrayList<>(); } - AtomicBoolean lockAcquired = new AtomicBoolean(false); - class AdminLockWatcher implements ServiceLock.AccumuloLockWatcher { @Override public void lostLock(ServiceLock.LockLossReason reason) { @@ -400,13 +397,11 @@ public void unableToMonitorLockNode(Exception e) { public void acquiredLock() { lockAcquiredLatch.countDown(); log.debug("Acquired ZooKeeper lock for Admin"); - lockAcquired.getAndSet(true); } @Override public void failedToAcquireLock(Exception e) { log.warn("Failed to acquire ZooKeeper lock for Admin, msg: " + e.getMessage()); - lockAcquired.getAndSet(false); } } diff --git a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java index 8a730211f2f..54b58eba4af 100644 --- a/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java +++ b/test/src/main/java/org/apache/accumulo/test/fate/TestLock.java @@ -20,7 +20,6 @@ import java.util.UUID; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.data.ResourceGroupId; import org.apache.accumulo.core.fate.user.UserFateStore; @@ -89,8 +88,6 @@ public ServiceLock createTestLock(ServerContext context) throws InterruptedExcep return lock; } - private final AtomicBoolean lockAcquired = new AtomicBoolean(false); - class TestLockWatcher implements ServiceLock.AccumuloLockWatcher { @Override @@ -107,13 +104,11 @@ public void unableToMonitorLockNode(Exception e) { public void acquiredLock() { lockAcquiredLatch.countDown(); log.debug("Acquired ZooKeeper lock for test"); - lockAcquired.getAndSet(true); } @Override public void failedToAcquireLock(Exception e) { log.warn("Failed to acquire ZooKeeper lock for test", e); - lockAcquired.getAndSet(false); } } } diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java index ea23a38a0c4..fa48aa1f1ff 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java @@ -23,7 +23,6 @@ import java.util.HashMap; import java.util.UUID; -import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.cli.ConfigOpts; import org.apache.accumulo.core.clientImpl.thrift.ClientService; @@ -145,8 +144,6 @@ public static void main(String[] args) throws Exception { metricsInfo.init(MetricsInfo.serviceTags(context.getInstanceName(), "zombie.server", serverPort.address, ResourceGroupId.DEFAULT)); - final AtomicBoolean acquiredLock = new AtomicBoolean(false); - AccumuloLockWatcher lw = new AccumuloLockWatcher() { @Override diff --git a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java index f60aaea14de..785eeacfcae 100644 --- a/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java +++ b/test/src/main/java/org/apache/accumulo/test/performance/NullTserver.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import java.util.UUID; -import java.util.concurrent.atomic.AtomicBoolean; import org.apache.accumulo.core.cli.ConfigOpts; import org.apache.accumulo.core.cli.Help; @@ -323,7 +322,6 @@ public static void main(String[] args) throws Exception { HostAndPort.fromParts(ConfigOpts.BIND_ALL_ADDRESSES, opts.port)); sa.startThriftServer("null tserver"); - AtomicBoolean acquiredLock = new AtomicBoolean(false); AccumuloLockWatcher miniLockWatcher = new AccumuloLockWatcher() { @Override @@ -339,13 +337,11 @@ public void unableToMonitorLockNode(Exception e) { @Override public void acquiredLock() { LOG.debug("Acquired ZooKeeper lock for NullTserver"); - acquiredLock.getAndSet(true); } @Override public void failedToAcquireLock(Exception e) { LOG.warn("Failed to acquire ZK lock for NullTserver, msg: " + e.getMessage()); - acquiredLock.getAndSet(false); } }; From d5616fad7df17477f1d6f2e1e0e0e4d8e19712db Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Mon, 29 Sep 2025 23:55:40 +0000 Subject: [PATCH 5/5] Remove final boolean and fix logger class --- .../org/apache/accumulo/core/lock/ServiceLockSupport.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java index f70e4936608..4f2c8d77d40 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java @@ -18,7 +18,6 @@ */ package org.apache.accumulo.core.lock; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.function.Supplier; @@ -149,8 +148,7 @@ public boolean cannotRetryLocking() { */ public static class ServiceLockWatcher implements AccumuloLockWatcher { - private final AtomicBoolean lockAcquired = new AtomicBoolean(false); - private final Logger log = LoggerFactory.getLogger(ServiceLock.class); + private final Logger log = LoggerFactory.getLogger(ServiceLockWatcher.class); private final Type server; private final Supplier shutdownComplete; private final Consumer lostLockAction; @@ -164,13 +162,12 @@ public ServiceLockWatcher(Type server, Supplier shutdownComplete, @Override public void acquiredLock() { - lockAcquired.getAndSet(true); + LOG.debug("Acquired {} lock", server); } @Override public void failedToAcquireLock(Exception e) { log.debug("Failed to acquire lock", e); - lockAcquired.getAndSet(false); } @Override