Rest Endpoint to Create ZNode #12497

suddendust · 2024-02-26T09:32:35Z

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.

Behaviour:

Access Option: 0x40,0x80 (PERSISTENT_WITH_TTL, PERSISTENT_SEQUENTIAL_WITH_TTL)
TTL: Applies.
Range: > 0, will return a 400 if access option is 0x40 or 0x80 and range < 0.

TTL does not apply in other cases. In such cases, there is no check on TTL.

This endpoint will return an Bad Request if a node already exists (with the corresponding error message).

Note: For TTL nodes, enable this System.setProperty("zookeeper.extendedTypesEnabled", "true") else it'll throw a KeeperException: UNIMPLEMENTED.

codecov-commenter · 2024-02-26T10:24:18Z

Codecov Report

Attention: Patch coverage is 9.09091% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 61.74%. Comparing base (59551e4) to head (bbf9d44).
Report is 31 commits behind head on master.

Files	Patch %	Lines
...ot/controller/api/resources/ZookeeperResource.java	0.00%	18 Missing ⚠️
...spi/utils/builder/ControllerRequestURLBuilder.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12497      +/-   ##
============================================
- Coverage     61.75%   61.74%   -0.01%     
  Complexity      207      207              
============================================
  Files          2436     2450      +14     
  Lines        133233   133550     +317     
  Branches      20636    20692      +56     
============================================
+ Hits          82274    82463     +189     
- Misses        44911    44985      +74     
- Partials       6048     6102      +54

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.69% <9.09%> (-0.02%)`	⬇️
java-21	`61.60% <9.09%> (-0.03%)`	⬇️
skip-bytebuffers-false	`61.73% <9.09%> (-0.02%)`	⬇️
skip-bytebuffers-true	`61.57% <9.09%> (+33.84%)`	⬆️
temurin	`61.74% <9.09%> (-0.01%)`	⬇️
unittests	`61.74% <9.09%> (-0.01%)`	⬇️
unittests1	`46.91% <0.00%> (+0.02%)`	⬆️
unittests2	`27.69% <9.09%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xiangfu0 · 2024-02-26T11:52:19Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+      data = payload;
+    }
+    if (StringUtils.isEmpty(data)) {
+      throw new ControllerApplicationException(LOGGER, "Must provide data through query parameter or payload",


I feel it's ok to create an empty znode. You can put a warning for this.

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.

xiangfu0 · 2024-02-26T11:58:09Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+    if (result) {
+      return new SuccessResponse("Successfully updated path: " + path);
+    } else {
+      throw new ControllerApplicationException(LOGGER, "ZNode already exists at path: " + path,


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?

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

Added explicit path check

xiangfu0 · 2024-02-26T12:09:37Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+      @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(


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.

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?

zhtaoxiang · 2024-02-26T17:39:12Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+          Response.Status.INTERNAL_SERVER_ERROR, e);
+    }
+    if (result) {
+      return new SuccessResponse("Successfully updated path: " + path);


nit: updated -> created

zhtaoxiang · 2024-02-26T17:43:59Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

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


can we please add an explanation: -1 means no ttl, and positive values means ephemeral znode

Ephemeral actually is controlled by the access option, not the ttl. TTLs only make sense for pesistent znodes.

Will add an explanation.

zhtaoxiang · 2024-02-26T17:45:58Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+
+    boolean result;
+    try {
+      result = _pinotHelixResourceManager.createZKNode(path, znRecord, accessOption, ttl);


I feel that we should check the range of ttl before making the call (or maybe check the range inside the createZKNode method)?

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.

What if the value is -2L? Will it treat it as not set?

With -2, node creation fails with an ISE, but I'll put a range check to make it clearer.

zhtaoxiang

LGTM

Jackie-Jiang · 2024-02-27T20:00:12Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+  })
+  public SuccessResponse createNode(
+      @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+      @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,


Why do we have both data and payload? Should it always be set with payload?

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.

I guess that keep payload only for now. We can add data if that is really useful, but removing it is much harder.

Jackie-Jiang · 2024-02-27T20:01:43Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+          Response.Status.BAD_REQUEST);
+    }
+    //check if node already exists
+    if (_pinotHelixResourceManager.getZKStat(path) != null) {


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

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:

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

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.

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

Yes, that's a good idea. Flipped it.

Jackie-Jiang · 2024-02-28T23:59:05Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+  })
+  public SuccessResponse createNode(
+      @ApiParam(value = "Zookeeper Path, must start with /", required = true) @QueryParam("path") String path,
+      @ApiParam(value = "Content") @QueryParam("data") @Nullable String data,


I guess that keep payload only for now. We can add data if that is really useful, but removing it is much harder.

Jackie-Jiang · 2024-02-29T00:04:22Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

+          Response.Status.BAD_REQUEST);
+    }
+    //check if node already exists
+    if (_pinotHelixResourceManager.getZKStat(path) != null) {


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

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java

…/resources/ZookeeperResource.java

suddendust added 3 commits February 26, 2024 14:54

Added rest endpoint to create znode at a path

c2d552a

Change error response message

106c5dd

Fix HTTP response code

6e03561

suddendust changed the title ~~Rest Endpoint for Create ZNode~~ Rest Endpoint to Create ZNode Feb 26, 2024

xiangfu0 requested review from Jackie-Jiang and snleee February 26, 2024 10:59

xiangfu0 added the rest-api label Feb 26, 2024

xiangfu0 reviewed Feb 26, 2024

View reviewed changes

zhtaoxiang reviewed Feb 26, 2024

View reviewed changes

suddendust added 7 commits February 26, 2024 23:27

Added explanation for ttl api param

5d1a7f9

Add API for zkCreate in ControllerRequestURLBuilder.java

f89416f

Change docs

5e93344

Added explicit path check

5b8f824

Check for PERSISTENT_SEQUENTIAL_WITH_TTL

40aec0b

Doc

18647cc

Improvement API param doc

6a16503

zhtaoxiang approved these changes Feb 27, 2024

View reviewed changes

Jackie-Jiang reviewed Feb 27, 2024

View reviewed changes

suddendust added 3 commits February 28, 2024 14:10

Format

361d314

WIP

62b60de

Merge branch 'master' of github.com:apache/pinot into create_znode_api

b52c75e

suddendust requested a review from Jackie-Jiang February 28, 2024 17:12

Jackie-Jiang approved these changes Feb 29, 2024

View reviewed changes

Address comments

bbf9d44

Jackie-Jiang reviewed Feb 29, 2024

View reviewed changes

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java Outdated Show resolved Hide resolved

Update pinot-controller/src/main/java/org/apache/pinot/controller/api…

6635d87

…/resources/ZookeeperResource.java

Jackie-Jiang merged commit 2249be3 into apache:master Feb 29, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest Endpoint to Create ZNode #12497

Rest Endpoint to Create ZNode #12497

suddendust commented Feb 26, 2024 •

edited

Loading

codecov-commenter commented Feb 26, 2024 •

edited

Loading

xiangfu0 Feb 26, 2024

suddendust Feb 26, 2024 •

edited

Loading

xiangfu0 Feb 26, 2024

suddendust Feb 26, 2024

suddendust Feb 27, 2024

xiangfu0 Feb 26, 2024

suddendust Feb 26, 2024

zhtaoxiang Feb 26, 2024

suddendust Feb 26, 2024

zhtaoxiang Feb 26, 2024

suddendust Feb 26, 2024 •

edited

Loading

suddendust Feb 26, 2024

suddendust Feb 26, 2024

zhtaoxiang Feb 26, 2024

suddendust Feb 26, 2024

zhtaoxiang Feb 26, 2024

suddendust Feb 27, 2024

zhtaoxiang left a comment

Jackie-Jiang Feb 27, 2024

suddendust Feb 28, 2024

Jackie-Jiang Feb 28, 2024

Jackie-Jiang Feb 27, 2024

suddendust Feb 28, 2024 •

edited

Loading

Jackie-Jiang Feb 29, 2024

suddendust Feb 29, 2024

Jackie-Jiang Feb 28, 2024

Jackie-Jiang Feb 29, 2024

Rest Endpoint to Create ZNode #12497

Rest Endpoint to Create ZNode #12497

Conversation

suddendust commented Feb 26, 2024 • edited Loading

codecov-commenter commented Feb 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

suddendust Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddendust Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhtaoxiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddendust Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddendust commented Feb 26, 2024 •

edited

Loading

codecov-commenter commented Feb 26, 2024 •

edited

Loading

suddendust Feb 26, 2024 •

edited

Loading

suddendust Feb 26, 2024 •

edited

Loading

suddendust Feb 28, 2024 •

edited

Loading