junkaixue commented on code in PR #2994: URL: https://github.com/apache/helix/pull/2994#discussion_r1917327288
########## zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java: ########## @@ -1820,6 +1826,66 @@ 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. 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 + */ + private List<Op> getOpsForRecursiveDelete(String root) { + List<Op> ops = new ArrayList<>(); + // Return early if the root does not exist + if (!exists(root)) { + return ops; + } + + Queue<String> nodes = new LinkedList<>(); + nodes.offer(root); + while (!nodes.isEmpty()) { + String node = nodes.poll(); + getChildren(node, false).stream().forEach(child -> nodes.offer(node + "/" + child)); + ops.add(Op.delete(node, -1)); + } + + Collections.reverse(ops); Review Comment: Add a comment here that this sorting is necessary. ########## zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java: ########## @@ -1820,6 +1826,66 @@ 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); + } Review Comment: I prefer not have branch of logics. This code can leverage deleteRecursivelyAtomic(List<String> paths) for implementation. like deleteRecursivelyAtomic(Arrays.asList(path)); -- 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