You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/07/30 19:35:34 UTC

[GitHub] [helix] xyuanlu commented on a change in pull request #1190: Add Helix rest Zookeeper delete API to allow removing ephemeral ZNode

xyuanlu commented on a change in pull request #1190:
URL: https://github.com/apache/helix/pull/1190#discussion_r463223224



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestZooKeeperAccessor.java
##########
@@ -192,4 +191,33 @@ public void testGetStat() throws IOException {
     // Clean up
     _testBaseDataAccessor.remove(path, AccessOption.PERSISTENT);
   }
+
+  @Test
+  public void testDelete() throws IOException {
+    String path = "/path";
+    String deletePath = path + "/delete";
+
+    try {
+      // 1. Create a persistent node. Delete shall fail.
+      _testBaseDataAccessor.create(deletePath, null, AccessOption.PERSISTENT);
+      // Verify with the REST endpoint
+      new JerseyUriRequestBuilder("zookeeper{}?command=delete").format(deletePath)
+          .expectedReturnStatusCode(Response.Status.FORBIDDEN.getStatusCode());
+      Assert.assertTrue(_testBaseDataAccessor.exists(deletePath, AccessOption.PERSISTENT));
+
+      // 2. Create a ephemeral node. Delete shall be done successfully.
+      _testBaseDataAccessor.remove(deletePath, AccessOption.PERSISTENT);
+      _testBaseDataAccessor.create(deletePath, null, AccessOption.EPHEMERAL);
+      // Verify with the REST endpoint
+      String data = new JerseyUriRequestBuilder("zookeeper{}?command=delete").format(deletePath)
+          .isBodyReturnExpected(true).get(this);
+      Map<String, String> result = OBJECT_MAPPER.readValue(data, HashMap.class);
+      Assert.assertEquals(result.get("path"), deletePath);
+      Assert.assertEquals(result.get("delete"), new Boolean(true).toString());
+      Assert.assertFalse(_testBaseDataAccessor.exists(deletePath, AccessOption.PERSISTENT));

Review comment:
       Question here (may not related to your change).
   Is the second param here in `exists(String path, int options)` useful? I did not find it being considered in any implementation of BaseDataAccessor.  




----------------------------------------------------------------
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.

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



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