pkuwm commented on a change in pull request #1190:
URL: https://github.com/apache/helix/pull/1190#discussion_r463901648
##########
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) {
+ Stat stat = zkBaseDataAccessor.getStat(path, AccessOption.PERSISTENT);
+ if (stat == null) {
+ return notFound();
Review comment:
Could we add a msg to this as well: ("Path %s does not exist", path)? I
think it gives a user a better idea. Otherwise the msg returned is unfriendly
if we use `curl endpoint` in terminal.
```
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 404 </title>
</head>
<body>
<h2>HTTP ERROR: 404</h2>
<p>Problem accessing /admin/v2/zookeeper/aa. Reason:
<pre> Not Found</pre></p>
<hr /><a href="http://eclipse.org/jetty";>Powered by Jetty://
9.4.12.v20180830</a><hr/>
</body>
</html>
```
VS
```
{
"message" : "Path /aa does not exist",
"status": 404
}
```
##########
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) {
+ 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();
Review comment:
At least add a message `OK("Success")`?
----------------------------------------------------------------
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]
