ZOOKEEPER-4504: ZKUtil#deleteRecursive causing deadlock in HDFS HA functionality

Make ZKUtil#deleteRecursive API fully compatible with older versions

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1843 from arshadmohammad/ZOOKEEPER-4504-DeleteRecursive and squashes the following commits:

851bb1ee9 [Mohammad Arshad] Added javadoc for ZKUtil#deleteRecursive(zk, pathRoot, batchSize) API
e7b33116c [Mohammad Arshad] Added test case to verify ZKUtil.deleteRecursive() in sync and async mode
008b2bd4a [Mohammad Arshad] ZOOKEEPER-4504: ZKUtil#deleteRecursive causing deadlock in HDFS HA functionality

2 files changed, 69 insertions, 3 deletions

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java
index d20634144..2ccdfc877 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java
@@ -49,7 +49,14 @@ public class ZKUtil {
      * If there is an error with deleting one of the sub-nodes in the tree,
      * this operation would abort and would be the responsibility of the app to handle the same.
      *
-     *
+     * @param zk Zookeeper client
+     * @param pathRoot path to be deleted
+     * @param batchSize number of delete operations to be submitted in one call.
+     *                  batchSize is also used to decide sync and async delete API invocation.
+     *                  If batchSize>0 then async otherwise sync delete API is invoked. batchSize>0
+     *                  gives better performance. batchSize<=0 scenario is handled to preserve
+     *                  backward compatibility.
+     * @return true if given node and all its sub nodes are deleted successfully otherwise false
      * @throws IllegalArgumentException if an invalid path is specified
      */
     public static boolean deleteRecursive(
@@ -61,7 +68,15 @@ public class ZKUtil {
         List<String> tree = listSubTreeBFS(zk, pathRoot);
         LOG.debug("Deleting tree: {}", tree);
 
-        return deleteInBatch(zk, tree, batchSize);
+        if (batchSize > 0) {
+            return deleteInBatch(zk, tree, batchSize);
+        } else {
+            for (int i = tree.size() - 1; i >= 0; --i) {
+                //Delete the leaves first and eventually get rid of the root
+                zk.delete(tree.get(i), -1); //Delete all versions of the node with -1.
+            }
+            return true;
+        }
     }
 
     /**
@@ -73,7 +88,8 @@ public class ZKUtil {
     public static void deleteRecursive(
         ZooKeeper zk,
         final String pathRoot) throws InterruptedException, KeeperException {
-        deleteRecursive(zk, pathRoot, 1000);
+        // batchSize=0 is passed to preserve the backward compatibility with older clients.
+        deleteRecursive(zk, pathRoot, 0);
     }
 
     private static class BatchedDeleteCbContext {
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/ZKUtilTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/ZKUtilTest.java
index 51df90702..a73ff2fa7 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/ZKUtilTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/ZKUtilTest.java
@@ -25,10 +25,12 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import java.io.File;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
+import org.apache.zookeeper.data.Stat;
 import org.apache.zookeeper.test.ClientBase;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -119,4 +121,52 @@ public class ZKUtilTest extends ClientBase {
         assertEquals(1, bList.size());
         assertIterableEquals(Collections.singletonList("/a/b"), bList);
     }
+
+    @Test
+    public void testDeleteRecursiveInAsyncMode() throws Exception {
+        int batchSize = 10;
+        testDeleteRecursiveInSyncAsyncMode(batchSize);
+    }
+
+    @Test
+    public void testDeleteRecursiveInSyncMode() throws Exception {
+        int batchSize = 0;
+        testDeleteRecursiveInSyncAsyncMode(batchSize);
+    }
+
+    // batchSize>0 is async mode otherwise it is sync mode
+    private void testDeleteRecursiveInSyncAsyncMode(int batchSize)
+        throws IOException, InterruptedException, KeeperException {
+        TestableZooKeeper zk = createClient();
+        String parentPath = "/a";
+        zk.create(parentPath, "".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+        int numberOfNodes = 50;
+        List<Op> ops = new ArrayList<>();
+        for (int i = 0; i < numberOfNodes; i++) {
+            ops.add(Op.create(parentPath + "/a" + i, "".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT));
+        }
+        zk.multi(ops);
+        ops.clear();
+
+        // check nodes create successfully
+        List<String> children = zk.getChildren(parentPath, false);
+        assertEquals(numberOfNodes, children.size());
+
+        // create one more level of z nodes
+        String subNode = "/a/a0";
+        for (int i = 0; i < numberOfNodes; i++) {
+            ops.add(Op.create(subNode + "/b" + i, "".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT));
+        }
+        zk.multi(ops);
+
+        // check sub nodes created successfully
+        children = zk.getChildren(subNode, false);
+        assertEquals(numberOfNodes, children.size());
+
+        ZKUtil.deleteRecursive(zk, parentPath, batchSize);
+        Stat exists = zk.exists(parentPath, false);
+        assertNull(exists, "ZKUtil.deleteRecursive() could not delete all the z nodes");
+    }
 }