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

[GitBox <gi...@apache.org>]

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



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
##########
@@ -196,6 +209,32 @@ private Response getStat(BaseDataAccessor<byte[]> zkBaseDataAccessor, String pat
     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) {

Review comment:
       I'm not seeing how this is any different from using delete. This is no better than using `delete` for two different types of delete's - delete and deleteEphemeral.
   
   Perhaps you could add a commandStr here to differentiate two different types of `delete`s, and when you want to add an endpoint for regular delete backed by ACL checks, then just implement that _if_ that becomes necessary?
   
   My point was not about what kind of REST verb we should use - it's pretty clear we should use DELETE in this case. But it's more about following a good API design which, again, is something that is hard to misuse by not embedding hidden assumptions or TODOs that may cause a behavior change down the road.




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