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? I don't think
this adds any more work/difficulty for the purposes of this PR? (If any, it
saves you the work of adding a TODO)
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.
Also, seen from another angle, supporting it as `deleteEphemeral` gives the
user a clear meaning to the command string as opposed to just calling it a HTTP
verb DELETE, which might leave the user confused and question the meaning of
the API when it fails to delete regular ZNodes.
You could add two commands, delete and deleteEphemeral, and make the default
commandStr delete, and throw a not authorized or not supported, and only let
deleteEphemeral go through. This way, there's no confusion when we do decide to
support delete operation with ACL, there's no confusion or change in behavior.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
