[helix] branch master updated: Add Helix rest Zookeeper delete API to allow removing ephemeral ZNode (#1190)

[ji...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jiajunwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git


The following commit(s) were added to refs/heads/master by this push:
     new b13d872  Add Helix rest Zookeeper delete API to allow removing ephemeral ZNode (#1190)
b13d872 is described below

commit b13d872bfc2711c10b90504caeea167ebc49257a
Author: Jiajun Wang <jj...@linkedin.com>
AuthorDate: Mon Aug 3 17:10:46 2020 -0700

    Add Helix rest Zookeeper delete API to allow removing ephemeral ZNode (#1190)
    
    Add a new Helix rest API in the ZookeeperAccessor for deleting an ephemeral ZNode.
    
    Note that before we have ACL/audit support in the Helix rest, allowing raw ZK write operation is dangerous.
    This API is introduced prematurely for resolving the issue of "zombie" participant (the instance has an active zk connection, but refuse to do any work). Currently, the existence of such a node may block the normal state transitions and then impact the cluster's availability. This PR restricts that only an ephemeral node can be deleted to minimize the risk.
---
 .../resources/zookeeper/ZooKeeperAccessor.java     | 51 +++++++++++++++++++---
 .../helix/rest/server/TestZooKeeperAccessor.java   | 30 ++++++++++++-
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
index b73c9bb..87e13e1 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
@@ -21,7 +21,7 @@ package org.apache.helix.rest.server.resources.zookeeper;
 
 import java.util.List;
 import java.util.Map;
-import javax.ws.rs.DefaultValue;
+import javax.ws.rs.DELETE;
 import javax.ws.rs.GET;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
@@ -47,7 +47,7 @@ import org.slf4j.LoggerFactory;
  * ZooKeeperAccessor provides methods for accessing ZooKeeper resources (ZNodes).
  * It provides basic ZooKeeper features supported by ZkClient.
  */
-@Path("/zookeeper")
+@Path("/zookeeper{path: /.+}")
 public class ZooKeeperAccessor extends AbstractResource {
   private static final Logger LOG = LoggerFactory.getLogger(ZooKeeperAccessor.class.getName());
   private BaseDataAccessor<byte[]> _zkBaseDataAccessor;
@@ -61,7 +61,6 @@ public class ZooKeeperAccessor extends AbstractResource {
   }
 
   @GET
-  @Path("{path: .+}")
   public Response get(@PathParam("path") String path, @QueryParam("command") String commandStr) {
     ZooKeeperCommand cmd = getZooKeeperCommandIfPresent(commandStr);
     if (cmd == null) {
@@ -73,9 +72,6 @@ public class ZooKeeperAccessor extends AbstractResource {
         (ServerContext) _application.getProperties().get(ContextPropertyKeys.SERVER_CONTEXT.name());
     _zkBaseDataAccessor = _serverContext.getByteArrayZkBaseDataAccessor();
 
-    // Need to prepend a "/" since JAX-RS regex removes it
-    path = "/" + path;
-
     // Check that the path supplied is valid
     if (!ZkValidationUtil.isPathValid(path)) {
       String errMsg = "The given path is not a valid ZooKeeper path: " + path;
@@ -101,6 +97,23 @@ public class ZooKeeperAccessor extends AbstractResource {
     }
   }
 
+  @DELETE
+  public Response delete(@PathParam("path") String path) {
+    // Lazily initialize ZkBaseDataAccessor
+    ServerContext _serverContext =
+        (ServerContext) _application.getProperties().get(ContextPropertyKeys.SERVER_CONTEXT.name());
+    _zkBaseDataAccessor = _serverContext.getByteArrayZkBaseDataAccessor();
+
+    // Check that the path supplied is valid
+    if (!ZkValidationUtil.isPathValid(path)) {
+      String errMsg = "The given path is not a valid ZooKeeper path: " + path;
+      LOG.info(errMsg);
+      return badRequest(errMsg);
+    }
+
+    return delete(_zkBaseDataAccessor, path);
+  }
+
   /**
    * Checks if a ZNode exists in the given path.
    * @param zkBaseDataAccessor
@@ -196,6 +209,32 @@ public class ZooKeeperAccessor extends AbstractResource {
     return JSONRepresentation(result);
   }
 
+  /**
+   * Delete the ZNode at the given path if exists.
+   * @param zkBaseDataAccessor
+   * @param path
+   * @return The delete result and the operated path.
+   */
+  private Response delete(BaseDataAccessor zkBaseDataAccessor, String path) {
+    Stat stat = zkBaseDataAccessor.getStat(path, AccessOption.PERSISTENT);
+    if (stat == null) {
+      return notFound();
+    } else if (stat.getEphemeralOwner() <= 0) {
+      // TODO: Remove this restriction once we have audit and ACL for the API calls.
+      // TODO: This method is added pre-maturely to support removing the live instance of a zombie
+      // TODO: instance. It is risky to allow all deleting requests before audit and ACL are done.
+      throw new WebApplicationException(Response.status(Response.Status.FORBIDDEN)
+          .entity(String.format("Deleting a non-ephemeral node is not allowed.")).build());
+    }
+
+    if (zkBaseDataAccessor.remove(path, AccessOption.PERSISTENT)) {
+      return OK();
+    } else {
+      throw new WebApplicationException(Response.status(Response.Status.INTERNAL_SERVER_ERROR)
+          .entity(String.format("Failed to delete %s.", path)).build());
+    }
+  }
+
   private ZooKeeperCommand getZooKeeperCommandIfPresent(String command) {
     return Enums.getIfPresent(ZooKeeperCommand.class, command).orNull();
   }
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestZooKeeperAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestZooKeeperAccessor.java
index 7d18c25..9515ce6 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestZooKeeperAccessor.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestZooKeeperAccessor.java
@@ -24,7 +24,6 @@ import java.util.Base64;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-
 import javax.ws.rs.core.Response;
 
 import org.apache.helix.AccessOption;
@@ -57,7 +56,7 @@ public class TestZooKeeperAccessor extends AbstractTestClass {
           throws ZkMarshallingError {
         return new String(bytes);
       }
-    });
+    }, ZkBaseDataAccessor.ZkClientType.DEDICATED);
   }
 
   @AfterClass
@@ -192,4 +191,31 @@ public class TestZooKeeperAccessor extends AbstractTestClass {
     // Clean up
     _testBaseDataAccessor.remove(path, AccessOption.PERSISTENT);
   }
+
+  @Test
+  public void testDelete() {
+    String path = "/path";
+    String deletePath = path + "/delete";
+
+    try {
+      // 1. Create a persistent node. Delete shall fail.
+      _testBaseDataAccessor.create(deletePath, null, AccessOption.PERSISTENT);
+      new JerseyUriRequestBuilder("zookeeper{}").format(deletePath)
+          .expectedReturnStatusCode(Response.Status.FORBIDDEN.getStatusCode()).delete(this);
+      Assert.assertTrue(_testBaseDataAccessor.exists(deletePath, AccessOption.PERSISTENT));
+      // 2. Try to delete a non-exist ZNode
+      new JerseyUriRequestBuilder("zookeeper{}").format(deletePath + "/foobar")
+          .expectedReturnStatusCode(Response.Status.NOT_FOUND.getStatusCode()).delete(this);
+      // 3. Create an ephemeral node. Delete shall be done successfully.
+      _testBaseDataAccessor.remove(deletePath, AccessOption.PERSISTENT);
+      _testBaseDataAccessor.create(deletePath, null, AccessOption.EPHEMERAL);
+      // Verify with the REST endpoint
+      new JerseyUriRequestBuilder("zookeeper{}").format(deletePath)
+          .expectedReturnStatusCode(Response.Status.OK.getStatusCode()).delete(this);
+      Assert.assertFalse(_testBaseDataAccessor.exists(deletePath, AccessOption.PERSISTENT));
+    } finally {
+      // Clean up
+      _testBaseDataAccessor.remove(path, AccessOption.PERSISTENT);
+    }
+  }
 }