HBASE-13925 Use zookeeper multi to clear znodes in ZKProcedureUtil

Signed-off-by: Andrew Purtell <apurtell@apache.org>

Conflicts:
	hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

@@ -1384,25 +1384,7 @@ public class ZKUtil {
    */
   public static void deleteNodeRecursively(ZooKeeperWatcher zkw, String node)
   throws KeeperException {
-    try {
+    deleteNodeRecursivelyMultiOrSequential(zkw, true, node);
-      List<String> children = ZKUtil.listChildrenNoWatch(zkw, node);
-      // the node is already deleted, so we just finish
-      if (children == null) return;
-
-      if(!children.isEmpty()) {
-        for(String child : children) {
-          deleteNodeRecursively(zkw, joinZNode(node, child));
-        }
-      }
-
-      //Zookeeper Watches are one time triggers; When children of parent nodes are deleted
-      //recursively, must set another watch, get notified of delete node
-      if (zkw.getRecoverableZooKeeper().exists(node, zkw) != null){
-        zkw.getRecoverableZooKeeper().delete(node, -1);
-      }
-    } catch(InterruptedException ie) {
-      zkw.interruptedException(ie);
-    }
   }
 
   /**
@@ -1476,6 +1458,69 @@ public class ZKUtil {
     }
   }
 
+  /**
+   * Delete the specified node and its children. This traverse the
+   * znode tree for listing the children and then delete
+   * these znodes including the parent using multi-update api or
+   * sequential based on the specified configurations.
+   * <p>
+   * Sets no watches. Throws all exceptions besides dealing with deletion of
+   * children.
+   * <p>
+   * If hbase.zookeeper.useMulti is true, use ZooKeeper's multi-update
+   * functionality. Otherwise, run the list of operations sequentially.
+   * <p>
+   * If all of the following are true:
+   * <ul>
+   * <li>runSequentialOnMultiFailure is true
+   * <li>hbase.zookeeper.useMulti is true
+   * </ul>
+   * on calling multi, we get a ZooKeeper exception that can be handled by a
+   * sequential call(*), we retry the operations one-by-one (sequentially).
+   *
+   * @param zkw
+   *          - zk reference
+   * @param runSequentialOnMultiFailure
+   *          - if true when we get a ZooKeeper exception that could retry the
+   *          operations one-by-one (sequentially)
+   * @param pathRoots
+   *          - path of the parent node(s)
+   * @throws KeeperException.NotEmptyException
+   *           if node has children while deleting
+   * @throws KeeperException
+   *           if unexpected ZooKeeper exception
+   * @throws IllegalArgumentException
+   *           if an invalid path is specified
+   */
+  public static void deleteNodeRecursivelyMultiOrSequential(ZooKeeperWatcher zkw,
+      boolean runSequentialOnMultiFailure, String... pathRoots) throws KeeperException {
+    if (pathRoots == null || pathRoots.length <= 0) {
+      LOG.warn("Given path is not valid!");
+      return;
+    }
+    List<ZKUtilOp> ops = new ArrayList<ZKUtil.ZKUtilOp>();
+    for (String eachRoot : pathRoots) {
+      // Zookeeper Watches are one time triggers; When children of parent nodes are deleted
+      // recursively, must set another watch, get notified of delete node
+      List<String> children = listChildrenBFSAndWatchThem(zkw, eachRoot);
+      // Delete the leaves first and eventually get rid of the root
+      for (int i = children.size() - 1; i >= 0; --i) {
+        ops.add(ZKUtilOp.deleteNodeFailSilent(children.get(i)));
+      }
+      try {
+        if (zkw.getRecoverableZooKeeper().exists(eachRoot, zkw) != null) {
+          ops.add(ZKUtilOp.deleteNodeFailSilent(eachRoot));
+        }
+      } catch (InterruptedException e) {
+        zkw.interruptedException(e);
+      }
+    }
+    // atleast one element should exist
+    if (ops.size() > 0) {
+      multiOrSequential(zkw, ops, runSequentialOnMultiFailure);
+    }
+  }
+
   /**
    * BFS Traversal of all the children under path, with the entries in the list,
    * in the same order as that of the traversal. Lists all the children without
@@ -1512,6 +1557,42 @@ public class ZKUtil {
     return tree;
   }
 
+  /**
+   * BFS Traversal of all the children under path, with the entries in the list,
+   * in the same order as that of the traversal.
+   * Lists all the children and set watches on to them.
+   *
+   * @param zkw
+   *          - zk reference
+   * @param znode
+   *          - path of node
+   * @return list of children znodes under the path
+   * @throws KeeperException
+   *           if unexpected ZooKeeper exception
+   */
+  private static List<String> listChildrenBFSAndWatchThem(ZooKeeperWatcher zkw, final String znode)
+      throws KeeperException {
+    Deque<String> queue = new LinkedList<String>();
+    List<String> tree = new ArrayList<String>();
+    queue.add(znode);
+    while (true) {
+      String node = queue.pollFirst();
+      if (node == null) {
+        break;
+      }
+      List<String> children = listChildrenAndWatchThem(zkw, node);
+      if (children == null) {
+        continue;
+      }
+      for (final String child : children) {
+        final String childPath = node + "/" + child;
+        queue.add(childPath);
+        tree.add(childPath);
+      }
+    }
+    return tree;
+  }
+
   /**
    * Represents an action taken by ZKUtil, e.g. createAndFailSilent.
    * These actions are higher-level than ZKOp actions, which represent

hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ZKProcedureUtil.java

@@ -267,29 +267,29 @@ public abstract class ZKProcedureUtil
   }
 
   public void clearChildZNodes() throws KeeperException {
-    // TODO This is potentially racy since not atomic. update when we support zk that has multi
     LOG.info("Clearing all procedure znodes: " + acquiredZnode + " " + reachedZnode + " "
         + abortZnode);
 
     // If the coordinator was shutdown mid-procedure, then we are going to lose
     // an procedure that was previously started by cleaning out all the previous state. Its much
     // harder to figure out how to keep an procedure going and the subject of HBASE-5487.
-    ZKUtil.deleteChildrenRecursively(watcher, acquiredZnode);
+    ZKUtil.deleteChildrenRecursivelyMultiOrSequential(watcher, true, acquiredZnode, reachedZnode,
-    ZKUtil.deleteChildrenRecursively(watcher, reachedZnode);
+      abortZnode);
-    ZKUtil.deleteChildrenRecursively(watcher, abortZnode);
   }
 
   public void clearZNodes(String procedureName) throws KeeperException {
-    // TODO This is potentially racy since not atomic. update when we support zk that has multi
     LOG.info("Clearing all znodes for procedure " + procedureName + "including nodes "
         + acquiredZnode + " " + reachedZnode + " " + abortZnode);
 
     // Make sure we trigger the watches on these nodes by creating them. (HBASE-13885)
-    ZKUtil.createAndFailSilent(watcher, getAcquiredBarrierNode(procedureName));
+    String acquiredBarrierNode = getAcquiredBarrierNode(procedureName);
-    ZKUtil.createAndFailSilent(watcher, getAbortZNode(procedureName));
+    String reachedBarrierNode = getReachedBarrierNode(procedureName);
+    String abortZNode = getAbortZNode(procedureName);
 
-    ZKUtil.deleteNodeRecursively(watcher, getAcquiredBarrierNode(procedureName));
+    ZKUtil.createAndFailSilent(watcher, acquiredBarrierNode);
-    ZKUtil.deleteNodeRecursively(watcher, getReachedBarrierNode(procedureName));
+    ZKUtil.createAndFailSilent(watcher, abortZNode);
-    ZKUtil.deleteNodeRecursively(watcher, getAbortZNode(procedureName));
+
+    ZKUtil.deleteNodeRecursivelyMultiOrSequential(watcher, true, acquiredBarrierNode,
+      reachedBarrierNode, abortZNode);
   }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKMulti.java

@@ -338,6 +338,80 @@ public class TestZKMulti {
     }
   }
 
+  /**
+   * Verifies that for the given root node, it should delete all the nodes recursively using
+   * multi-update api.
+   */
+  @Test(timeout = 60000)
+  public void testDeleteNodeRecursivelyMulti() throws Exception {
+    String parentZNode = "/testdeleteNodeRecursivelyMulti";
+    createZNodeTree(parentZNode);
+
+    ZKUtil.deleteNodeRecursively(zkw, parentZNode);
+    assertTrue("Parent znode should be deleted.", ZKUtil.checkExists(zkw, parentZNode) == -1);
+  }
+
+  /**
+   * Verifies that for the given root node, it should delete all the nodes recursively using
+   * normal sequential way.
+   */
+  @Test(timeout = 60000)
+  public void testDeleteNodeRecursivelySequential() throws Exception {
+    String parentZNode = "/testdeleteNodeRecursivelySequential";
+    createZNodeTree(parentZNode);
+    boolean useMulti = zkw.getConfiguration().getBoolean("hbase.zookeeper.useMulti", false);
+    zkw.getConfiguration().setBoolean("hbase.zookeeper.useMulti", false);
+    try {
+      // disables the multi-update api execution
+      ZKUtil.deleteNodeRecursively(zkw, parentZNode);
+      assertTrue("Parent znode should be deleted.", ZKUtil.checkExists(zkw, parentZNode) == -1);
+    } finally {
+      // sets back the multi-update api execution
+      zkw.getConfiguration().setBoolean("hbase.zookeeper.useMulti", useMulti);
+    }
+  }
+
+  @Test(timeout = 60000)
+  public void testDeleteNodeRecursivelyMultiOrSequential() throws Exception {
+    String parentZNode1 = "/testdeleteNode1";
+    String parentZNode2 = "/testdeleteNode2";
+    String parentZNode3 = "/testdeleteNode3";
+    createZNodeTree(parentZNode1);
+    createZNodeTree(parentZNode2);
+    createZNodeTree(parentZNode3);
+
+    ZKUtil.deleteNodeRecursivelyMultiOrSequential(zkw, false, parentZNode1, parentZNode2,
+      parentZNode3);
+    assertTrue("Parent znode 1 should be deleted.", ZKUtil.checkExists(zkw, parentZNode1) == -1);
+    assertTrue("Parent znode 2 should be deleted.", ZKUtil.checkExists(zkw, parentZNode2) == -1);
+    assertTrue("Parent znode 3 should be deleted.", ZKUtil.checkExists(zkw, parentZNode3) == -1);
+  }
+
+  @Test(timeout = 60000)
+  public void testDeleteChildrenRecursivelyMultiOrSequential() throws Exception {
+    String parentZNode1 = "/testdeleteChildren1";
+    String parentZNode2 = "/testdeleteChildren2";
+    String parentZNode3 = "/testdeleteChildren3";
+    createZNodeTree(parentZNode1);
+    createZNodeTree(parentZNode2);
+    createZNodeTree(parentZNode3);
+
+    ZKUtil.deleteChildrenRecursivelyMultiOrSequential(zkw, true, parentZNode1, parentZNode2,
+      parentZNode3);
+
+    assertTrue("Wrongly deleted parent znode 1!", ZKUtil.checkExists(zkw, parentZNode1) > -1);
+    List<String> children = zkw.getRecoverableZooKeeper().getChildren(parentZNode1, false);
+    assertTrue("Failed to delete child znodes of parent znode 1!", 0 == children.size());
+
+    assertTrue("Wrongly deleted parent znode 2!", ZKUtil.checkExists(zkw, parentZNode2) > -1);
+    children = zkw.getRecoverableZooKeeper().getChildren(parentZNode2, false);
+    assertTrue("Failed to delete child znodes of parent znode 1!", 0 == children.size());
+
+    assertTrue("Wrongly deleted parent znode 3!", ZKUtil.checkExists(zkw, parentZNode3) > -1);
+    children = zkw.getRecoverableZooKeeper().getChildren(parentZNode3, false);
+    assertTrue("Failed to delete child znodes of parent znode 1!", 0 == children.size());
+  }
+
   private void createZNodeTree(String rootZNode) throws KeeperException,
       InterruptedException {
     List<Op> opList = new ArrayList<Op>();