From 0b953c5ca450c0b1fd41aa6538e4c83797b32de6 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Fri, 9 Jan 2026 20:12:32 +0000 Subject: [PATCH 1/2] Consolidates the ServerLock deletion code Modifies ZooZap to use the same deletion logic as ServiceLock. --- .../core/fate/zookeeper/ServiceLock.java | 29 +++++++++++++++++++ .../apache/accumulo/server/util/ZooZap.java | 20 ++++--------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java index 42eedec758e..8d7e05cfe04 100644 --- a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java @@ -23,10 +23,13 @@ import java.util.ArrayList; import java.util.List; import java.util.UUID; +import java.util.function.Consumer; +import java.util.function.Predicate; import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; import org.apache.accumulo.core.fate.zookeeper.ZooUtil.LockID; import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy; +import org.apache.accumulo.core.util.HostAndPort; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.KeeperException.Code; @@ -716,6 +719,32 @@ public long getSessionId() throws KeeperException, InterruptedException { } } + /** + * This method will delete multiple server locks for a given path according the predicate + * conditions. + * + * @param hostPortPredicate conditional predicate for determining if the lock should be removed. + * @param messageOutput function for setting where the output from the lockPath goes + * @param dryRun allows lock format validation and the messageOutput to be sent without actually + * deleting the lock + * + */ + public static void deleteLocks(ZooReaderWriter zk, String zPath, + Predicate hostPortPredicate, Consumer messageOutput, Boolean dryRun) + throws KeeperException, InterruptedException { + if (zk.exists(zPath)) { + List children = zk.getChildren(zPath); + for (String child : children) { + if (hostPortPredicate.test(HostAndPort.fromString(child))) { + messageOutput.accept("Deleting " + zPath + "/" + child + " from zookeeper"); + if (!dryRun) { + deleteLock(zk, path(child)); + } + } + } + } + } + public static void deleteLock(ZooReaderWriter zk, ServiceLockPath path) throws InterruptedException, KeeperException { diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java b/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java index 3d548c37cf9..3f8d1c0d6b0 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java @@ -206,7 +206,8 @@ public void zap(SiteConfiguration siteConf, String... args) { zoo.recursiveDelete(tserversPath + "/" + child, NodeMissingPolicy.SKIP); } } else { - removeLocks(zoo, tserversPath, hostPortPredicate, opts); + ServiceLock.deleteLocks(zoo, tserversPath, hostPortPredicate, m -> message(m, opts), + opts.dryRun); } } catch (KeeperException | InterruptedException e) { log.error("Error deleting tserver locks", e); @@ -269,7 +270,8 @@ static void removeGroupedLocks(ZooReaderWriter zoo, String path, Predicate groups = zoo.getChildren(path); for (String group : groups) { if (groupPredicate.test(group)) { - removeLocks(zoo, path + "/" + group, hostPortPredicate, opts); + ServiceLock.deleteLocks(zoo, path + "/" + group, hostPortPredicate, m -> message(m, opts), + opts.dryRun); } } } @@ -278,19 +280,7 @@ static void removeGroupedLocks(ZooReaderWriter zoo, String path, Predicate hostPortPredicate, Opts opts) throws KeeperException, InterruptedException { - if (zoo.exists(path)) { - List children = zoo.getChildren(path); - for (String child : children) { - if (hostPortPredicate.test(HostAndPort.fromString(child))) { - message("Deleting " + path + "/" + child + " from zookeeper", opts); - if (!opts.dryRun) { - // TODO not sure this is the correct way to delete this lock.. the code was deleting - // locks in multiple different ways for diff servers types. - zoo.recursiveDelete(path + "/" + child, NodeMissingPolicy.SKIP); - } - } - } - } + ServiceLock.deleteLocks(zoo, path, hostPortPredicate, m -> message(m, opts), opts.dryRun); } static void removeSingletonLock(ZooReaderWriter zoo, String path, From df696473538b790ede33f383d85d3db225c8e0d1 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Tue, 20 Jan 2026 14:59:23 +0000 Subject: [PATCH 2/2] Adds input validation and removes all locks Adds input path validation to deleteLocks and ensures that all locks are removed for a given server location. Cannot use deleteLock as that only deletes the first child lock for a given path. --- .../core/fate/zookeeper/ServiceLock.java | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java index 8d7e05cfe04..42e57206e58 100644 --- a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.UUID; import java.util.function.Consumer; import java.util.function.Predicate; @@ -720,8 +721,7 @@ public long getSessionId() throws KeeperException, InterruptedException { } /** - * This method will delete multiple server locks for a given path according the predicate - * conditions. + * This method will delete all server locks for a given path according the predicate conditions. * * @param hostPortPredicate conditional predicate for determining if the lock should be removed. * @param messageOutput function for setting where the output from the lockPath goes @@ -732,19 +732,36 @@ public long getSessionId() throws KeeperException, InterruptedException { public static void deleteLocks(ZooReaderWriter zk, String zPath, Predicate hostPortPredicate, Consumer messageOutput, Boolean dryRun) throws KeeperException, InterruptedException { - if (zk.exists(zPath)) { - List children = zk.getChildren(zPath); - for (String child : children) { - if (hostPortPredicate.test(HostAndPort.fromString(child))) { - messageOutput.accept("Deleting " + zPath + "/" + child + " from zookeeper"); - if (!dryRun) { - deleteLock(zk, path(child)); - } + + Objects.requireNonNull(zPath, "Lock path cannot be null"); + if (!zk.exists(zPath)) { + throw new IllegalStateException("Path " + zPath + " does not exist"); + } + + List servers = zk.getChildren(zPath); + if (servers.isEmpty()) { + throw new IllegalStateException("No server locks are held at " + zPath); + } + + for (String server : servers) { + if (hostPortPredicate.test(HostAndPort.fromString(server))) { + messageOutput.accept("Deleting " + zPath + "/" + server + " from zookeeper"); + if (!dryRun) { + LOG.debug("Deleting all locks at path {} due to lock deletion", zPath); + zk.recursiveDelete(zPath + "/" + server, NodeMissingPolicy.SKIP); } } } } + /** + * This method will delete the top server lock for a given lock path + * + * @param zk zookeeper client + * @param path path for lock deletion only the top child lock will be removed + * + */ + public static void deleteLock(ZooReaderWriter zk, ServiceLockPath path) throws InterruptedException, KeeperException {