You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "suddendust (via GitHub)" <gi...@apache.org> on 2024/02/26 09:32:38 UTC
[PR] Rest Endpoint for Create ZNode [pinot]
suddendust opened a new pull request, #12497:
URL: https://github.com/apache/pinot/pull/12497
The only way to create a znode right now via the controller API is using `/zk/put`. However, this overwrites any previous data already existing at the given path. There are certain use-cases in which we want to fail creation of a node if it already exists. For example, creating an ephemeral znode at a path to acquire a mutex. For such cases, we want to create the lock atomically.
This endpoint will return an ISE if a node already exists (with the corresponding error message).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503059811
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
Review Comment:
I feel that we should check the range of ttl before making the call (or maybe check the range inside the createZKNode method)?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
Review Comment:
can we please add an explanation: -1 means no ttl, and positive values means ephemeral znode
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
Review Comment:
nit: updated -> created
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503068074
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
Review Comment:
Ephemeral actually is controlled by the access option, not the ttl. TTLs only make sense for pesistent znodes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1505373850
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
This was put in to explicitly detect if a path already exists. ZK does not return any error codes to detect it. Yes, a race condition exists, but that should be fine. Two cases:
1. This check passes but node exists by the time ZK tries to create this node: It'll fail anyway transparently to the user (with an ISE instead of Bad Request though, which should be okay as the user would retry).
2. This check fails but node is deleted by another process before user receives a response: This is possible even if we leave this check to ZK.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1505363338
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
Review Comment:
This signature was just copied from the `zk/put` API as I want to keep them similar. But we can def do with just payload.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503076566
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
Review Comment:
Done
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
Review Comment:
Done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1508023342
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,67 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or just ignore it", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1")
+ int ttl, @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ //validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+
+ if (StringUtils.isEmpty(payload)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
Review Comment:
```suggestion
throw new ControllerApplicationException(LOGGER, "Must provide payload", Response.Status.BAD_REQUEST);
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1505373850
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
This was put in to explicitly detect if a path already exists. ZK does not return any error codes to detect it. Yes, a race condition exists, but that should be fine. Two cases:
1. This check passes but node exists by the time ZK tries to create this node: It'll fail anyway transparently to the user.
2. This check fails but node is deleted by another process before user receives a response: This is possible even if we leave this check to ZK.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502963805
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
+ } else {
+ throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,
Review Comment:
` _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);` returns a false if the node already exists. I don't want to check for path first because there can be a race condition in the compound check-then-act action - I want this operation to be atomic.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503692048
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
+ } else {
+ throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,
Review Comment:
Added explicit path check
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503074799
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
Review Comment:
There isn't a range of tll specified in the zk client. -1L means it's not set, any positive value means it's the TTL.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1504897646
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
Do we need this check? Will `createZKNode()` throw exception when the node already exists? We should rely on ZK to handle conflict, or there will be potential race condition
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
Review Comment:
Why do we have both `data` and `payload`? Should it always be set with `payload`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12497:
URL: https://github.com/apache/pinot/pull/12497
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1507874579
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
Yes, that's a good idea. Flipped it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502963805
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
+ } else {
+ throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,
Review Comment:
` _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);` returns a false if the node already exists. I don't want to check for path first because there can be a race condition in the compound check-then-act action - I want this operation to be atomic.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502966292
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
+ } else {
+ throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,
Review Comment:
` _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);` returns a false if the node already exists, can't think of other scenarios in which it would return a false. But sure we can check for the existence of the path first.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1505373850
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
This was put in to explicitly detect if a path already exists. ZK does not return any error codes to detect it. Yes, a race condition exists, but that should be fine. Two cases:
1. This check passed but node exists by the time ZK tries to create this node: It'll fail anyway transparently to the user.
2. This check fails but node is deleted by another process before user receives a response: This is possible even if we leave this check to ZK.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503068074
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
Review Comment:
Ephemeral actually is controlled by the access option, not the ttl.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
Review Comment:
Will add an explanation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502967065
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
Review Comment:
I'd not want to change the behaviour of the existing API right now.
Edit: I mean if we are making changes to the existing `/zk/put` API.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502968851
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
Review Comment:
This complicates the existing API. Now we have 1. `expectedVersion` which is not required for creating new node. 2. `ttl` which is not required to update a new node. Not to mention the POST vs PUT semantics would also be incorrect. Wdyt?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1506815769
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
Review Comment:
I guess that keep `payload` only for now. We can add `data` if that is really useful, but removing it is much harder.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +258,71 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "TTL of the node. TTL are only honoured for persistent znodes (access option = 0x40 or 0x80),"
+ + " in which case TTL should be > 0. If access option is not 0x40 or 0x80, it will be ignored, and we can "
+ + "set it to any value, or be ignored", defaultValue = "-1")
+ @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+// validate ttl range
+ if ((accessOption == AccessOption.PERSISTENT_WITH_TTL
+ || accessOption == AccessOption.PERSISTENT_SEQUENTIAL_WITH_TTL) && ttl <= 0) {
+ throw new ControllerApplicationException(LOGGER, "TTL for persistent nodes should be > 0",
+ Response.Status.BAD_REQUEST);
+ }
+ //check if node already exists
+ if (_pinotHelixResourceManager.getZKStat(path) != null) {
Review Comment:
How about we reverse this check? When creation fails, we check if it exists. This should give better handling, and in most cases we can reduce one ZK access
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503291081
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
Review Comment:
What if the value is -2L? Will it treat it as not set?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1503665318
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
Review Comment:
With -2, node creation fails with an ISE, but I'll put a range check to make it clearer.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12497:
URL: https://github.com/apache/pinot/pull/12497#issuecomment-1963780351
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `17 lines` in your changes are missing coverage. Please review.
> Project coverage is 61.61%. Comparing base [(`fe75309`)](https://app.codecov.io/gh/apache/pinot/commit/fe7530969dc850fc46e8346732d2695e73bdd09a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6e03561`)](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 16 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ot/controller/api/resources/ZookeeperResource.java](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1pvb2tlZXBlclJlc291cmNlLmphdmE=) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12497 +/- ##
============================================
- Coverage 61.72% 61.61% -0.11%
Complexity 207 207
============================================
Files 2436 2436
Lines 133219 133252 +33
Branches 20635 20642 +7
============================================
- Hits 82233 82108 -125
- Misses 44940 45089 +149
- Partials 6046 6055 +9
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.61% <0.00%> (+<0.01%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.58% <0.00%> (-0.13%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.74% <0.00%> (-33.85%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.61% <0.00%> (-0.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.61% <0.00%> (-0.11%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.69% <ø> (-0.17%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12497/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.74% <0.00%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12497?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502493581
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
Review Comment:
I feel it's ok to create an empty znode. You can put a warning for this.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
Review Comment:
This is pretty much the same as putData API, except ttl option.
Should you add a new parameter like `force` default to `true` into `putData` API instead of creating a new one?
You can add a new exception into it like `NodeAlreadyExist`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
+ Response.Status.BAD_REQUEST);
+ }
+ ZNRecord znRecord;
+ try {
+ znRecord = MAPPER.readValue(data, ZNRecord.class);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize the data", Response.Status.BAD_REQUEST,
+ e);
+ }
+
+ boolean result;
+ try {
+ result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to create znode at path: " + path,
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ if (result) {
+ return new SuccessResponse("Successfully updated path: " + path);
+ } else {
+ throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,
Review Comment:
how do you know is due to znode already exists?
Shall we check the znode existence first then throw or check znode exist inside the else then throw?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rest Endpoint to Create ZNode [pinot]
Posted by "suddendust (via GitHub)" <gi...@apache.org>.
suddendust commented on code in PR #12497:
URL: https://github.com/apache/pinot/pull/12497#discussion_r1502967065
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java:
##########
@@ -256,6 +257,56 @@ public SuccessResponse putData(
}
}
+ @POST
+ @Path("/zk/create")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.UPDATE_ZNODE)
+ @Authenticate(AccessType.CREATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Create a node at a given path")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse createNode(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+ @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,
+ @ApiParam(value = "ttl", defaultValue = "-1") @QueryParam("ttl") @DefaultValue("-1") int ttl,
+ @ApiParam(value = "accessOption", defaultValue = "1") @QueryParam("accessOption") @DefaultValue("1")
+ int accessOption, @Nullable String payload) {
+
+ path = validateAndNormalizeZKPath(path, false);
+
+ if (StringUtils.isEmpty(data)) {
+ data = payload;
+ }
+ if (StringUtils.isEmpty(data)) {
+ throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",
Review Comment:
I'd not want to change the behaviour of the existing API right now.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org