junkaixue commented on code in PR #2994:
URL: https://github.com/apache/helix/pull/2994#discussion_r1913855031


##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java:
##########
@@ -49,6 +49,7 @@
 import org.testng.Assert;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.Test;
+import org.apache.zookeeper.Op;

Review Comment:
   No code change why need to add this?



##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -274,18 +273,14 @@ public void dropInstance(String clusterName, 
InstanceConfig instanceConfig) {
           "Node " + instanceName + " is still alive for cluster " + 
clusterName + ", can't drop.");
     }
 
-    // delete config path
-    String instanceConfigsPath = 
PropertyPathBuilder.instanceConfig(clusterName);
-    ZKUtil.dropChildren(_zkClient, instanceConfigsPath, 
instanceConfig.getRecord());
-    // delete instance path
-    dropInstancePathRecursively(instancePath, 
instanceConfig.getInstanceName());
+    dropInstancePathsRecursively(instanceName, instancePath, 
instanceConfigPath);

Review Comment:
   I dont think we allow instance config name under config different from what 
stays in INSTANCE folder. This is required in Helix.
   
   So delete it by default assumption without give flexibility to add path 
unless it is general delete with extra path.



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1820,6 +1825,74 @@ public void deleteRecursively(String path) throws 
ZkClientException {
     }
   }
 
+  /**
+   * Delete the path as well as all its children. This operation is atomic and 
will either delete all nodes or none.
+   * This operation may fail if another agent is concurrently creating or 
deleting nodes under the path.
+   * @param path ZK path to delete
+   */
+  public void deleteRecursivelyAtomic(String path) {
+    List<Op> ops = getOpsForRecursiveDelete(path);
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete {}, exception {}", _uid, path, 
e);
+      throw new ZkClientException("Failed to delete " + path, e);
+    }
+  }
+
+  /**
+   * Delete all provided paths as well as all their children. This operation 
is atomic and will either delete all nodes
+   * or none. This operation may fail if another agent is concurrently 
creating or deleting nodes under any of the paths
+   * @param paths ZK paths to delete
+   */
+  public void deleteRecursivelyAtomic(List<String> paths) {
+    List<Op> ops = new ArrayList<>();
+    for (String path : paths) {
+      ops.addAll(getOpsForRecursiveDelete(path));
+    }
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete paths {}, exception {}", _uid, 
paths, e);
+      throw new ZkClientException("Failed to delete paths " + paths, e);
+    }
+  }
+
+  /**
+   * 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.
+   * @param root the root node to delete
+   * @return the list of ZK operations to delete the given root and all its 
children
+   */
+  private List<Op> getOpsForRecursiveDelete(String root) {
+    List<Op> ops = new ArrayList<>();
+    // Return early if the root does not exist
+    if (!exists(root)) {
+      return ops;
+    }
+
+    HashSet<String> visited = new HashSet<>();
+    Stack<String> nodes = new Stack<>();
+    nodes.push(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));

Review Comment:
   Also a question here. If the parent node add first, will it cause problem 
with deletion failed...



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1820,6 +1825,74 @@ public void deleteRecursively(String path) throws 
ZkClientException {
     }
   }
 
+  /**
+   * Delete the path as well as all its children. This operation is atomic and 
will either delete all nodes or none.
+   * This operation may fail if another agent is concurrently creating or 
deleting nodes under the path.
+   * @param path ZK path to delete
+   */
+  public void deleteRecursivelyAtomic(String path) {
+    List<Op> ops = getOpsForRecursiveDelete(path);
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete {}, exception {}", _uid, path, 
e);
+      throw new ZkClientException("Failed to delete " + path, e);
+    }
+  }
+
+  /**
+   * Delete all provided paths as well as all their children. This operation 
is atomic and will either delete all nodes
+   * or none. This operation may fail if another agent is concurrently 
creating or deleting nodes under any of the paths
+   * @param paths ZK paths to delete
+   */
+  public void deleteRecursivelyAtomic(List<String> paths) {
+    List<Op> ops = new ArrayList<>();
+    for (String path : paths) {
+      ops.addAll(getOpsForRecursiveDelete(path));
+    }
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete paths {}, exception {}", _uid, 
paths, e);
+      throw new ZkClientException("Failed to delete paths " + paths, e);
+    }
+  }
+
+  /**
+   * 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.
+   * @param root the root node to delete
+   * @return the list of ZK operations to delete the given root and all its 
children
+   */
+  private List<Op> getOpsForRecursiveDelete(String root) {
+    List<Op> ops = new ArrayList<>();
+    // Return early if the root does not exist
+    if (!exists(root)) {
+      return ops;
+    }
+
+    HashSet<String> visited = new HashSet<>();

Review Comment:
   If it is a tree not a cycled graph and BFS does not require visited to 
check....



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1820,6 +1825,74 @@ public void deleteRecursively(String path) throws 
ZkClientException {
     }
   }
 
+  /**
+   * Delete the path as well as all its children. This operation is atomic and 
will either delete all nodes or none.
+   * This operation may fail if another agent is concurrently creating or 
deleting nodes under the path.
+   * @param path ZK path to delete
+   */
+  public void deleteRecursivelyAtomic(String path) {
+    List<Op> ops = getOpsForRecursiveDelete(path);
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete {}, exception {}", _uid, path, 
e);
+      throw new ZkClientException("Failed to delete " + path, e);
+    }
+  }
+
+  /**
+   * Delete all provided paths as well as all their children. This operation 
is atomic and will either delete all nodes
+   * or none. This operation may fail if another agent is concurrently 
creating or deleting nodes under any of the paths
+   * @param paths ZK paths to delete
+   */
+  public void deleteRecursivelyAtomic(List<String> paths) {
+    List<Op> ops = new ArrayList<>();
+    for (String path : paths) {
+      ops.addAll(getOpsForRecursiveDelete(path));
+    }
+    try {
+      multi(ops);
+    }
+    catch (Exception e) {
+      LOG.error("zkclient {}, Failed to delete paths {}, exception {}", _uid, 
paths, e);
+      throw new ZkClientException("Failed to delete paths " + paths, e);
+    }
+  }
+
+  /**
+   * 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.
+   * @param root the root node to delete
+   * @return the list of ZK operations to delete the given root and all its 
children
+   */
+  private List<Op> getOpsForRecursiveDelete(String root) {
+    List<Op> ops = new ArrayList<>();
+    // Return early if the root does not exist
+    if (!exists(root)) {
+      return ops;
+    }
+
+    HashSet<String> visited = new HashSet<>();
+    Stack<String> nodes = new Stack<>();
+    nodes.push(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);

Review Comment:
   Too messy...
   
   String node = nodes.pop();
   List<String> children = getChildren(node, false);
   if (children.isEmpty()) {
      ops.add(Op.delete(node, -1));
      continue;
   }
   
   children.stream().foreach(c -> nodes.push(node + "/" + c);
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org

Reply via email to