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/08/01 01:24:16 UTC
[GitHub] [helix] pkuwm commented on a change in pull request #1190: Add Helix rest Zookeeper delete API to allow removing ephemeral ZNode
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org