respond reviewer feedback

apache · Jan 14, 2025 · dc99ee2 · dc99ee2
1 parent 03e1cd3
commit dc99ee2
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 25 deletions.
diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
@@ -273,10 +273,12 @@ public void dropInstance(String clusterName, InstanceConfig instanceConfig) {
           "Node " + instanceName + " is still alive for cluster " + clusterName + ", can't drop.");
     }
 
-    dropInstancePathsRecursively(instanceName, instancePath, instanceConfigPath);
+    dropInstancePathsRecursively(clusterName, instanceName);
   }
 
-  private void dropInstancePathsRecursively(String instanceName, String instancePath, String instanceConfigPath) {
+  private void dropInstancePathsRecursively(String clusterName, String instanceName) {
+    String instanceConfigPath = PropertyPathBuilder.instanceConfig(clusterName, instanceName);
+    String instancePath = PropertyPathBuilder.instance(clusterName, instanceName);
     int retryCnt = 0;
     while (true) {
       try {
@@ -328,9 +330,7 @@ public void purgeOfflineInstances(String clusterName, long offlineDuration) {
 
   private void purgeInstance(String clusterName, String instanceName) {
     logger.info("Purge instance {} from cluster {}.", instanceName, clusterName);
-    String instanceConfigPath = PropertyPathBuilder.instanceConfig(clusterName, instanceName);
-    String instancePath = PropertyPathBuilder.instance(clusterName, instanceName);
-    dropInstancePathsRecursively(instanceName, instancePath, instanceConfigPath);
+    dropInstancePathsRecursively(clusterName, instanceName);
   }
 
   @Override

diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java
@@ -49,7 +49,6 @@
 import org.testng.Assert;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.Test;
-import org.apache.zookeeper.Op;
 
 public class TestZkBaseDataAccessor extends ZkUnitTestBase {
   // serialize/deserialize integer list to byte array

diff --git a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
@@ -22,14 +22,15 @@
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Date;
-import java.util.HashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.OptionalLong;
+import java.util.Queue;
 import java.util.Set;
-import java.util.Stack;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArraySet;
@@ -1861,8 +1862,8 @@ public void deleteRecursivelyAtomic(List<String> paths) {
   }
 
   /**
-   * Get the list of operations to delete the given root and all its children. Performs simple BFS to put delete
-   * operations for leaf nodes first before parent nodes.
+   * Get the list of operations to delete the given root and all its children. Ops will be ordered so that deletion of
+   * children will come before parent nodes.
    * @param root the root node to delete
    * @return the list of ZK operations to delete the given root and all its children
    */
@@ -1873,26 +1874,19 @@ private List<Op> getOpsForRecursiveDelete(String root) {
       return ops;
     }
 
-    HashSet<String> visited = new HashSet<>();
-    Stack<String> nodes = new Stack<>();
-    nodes.push(root);
-
+    Queue<String> nodes = new LinkedList<>();
+    nodes.offer(root);
     while (!nodes.isEmpty()) {
-      String node = nodes.peek();
-      List<String> children = getChildren(node, false);
-      if (children.isEmpty() || visited.contains(node)) {
-        nodes.pop();
-        ops.add(Op.delete(node, -1));
-      } else {
-        for (String child : children) {
-          nodes.push(node + "/" + child);
-        }
-      }
-      visited.add(node);
+      String node = nodes.poll();
+      getChildren(node, false).stream().forEach(child -> nodes.offer(node + "/" + child));
+      ops.add(Op.delete(node, -1));
     }
+
+    Collections.reverse(ops);
     return ops;
   }
 
+
   private void processDataOrChildChange(WatchedEvent event, long notificationTime) {
     final String path = event.getPath();
     final boolean pathExists = event.getType() != EventType.NodeDeleted;