Add method to create full key path if it does not exist #2607

GrantPSpencer · 2023-08-23T19:45:33Z

Issues

My PR addresses the following Helix issues and references them in the PR description:

Metaclient currently does not offer the functionality of attempting to create a key's parents if they do not exist.
I'd like to expose a method that does the following:
Assume no nodes have been created
createFullPath("/a/b/c/d", data)
This would then create nodes /a, /b, and /c with null values and lastly create node /d with the data passed in.

Description

Here are some details about my PR, including screenshots of any UI changes:

Add functionality to create the full path to a key if the path does not already exist.

Tests

The following tests are written for this issue:

Added unit tests to both TestStressZkClient.Java and TestZkMetaClient.java

Changes that Break Backward Compatibility (Optional)

My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:

None. Only new method

Documentation (Optional)

In case of new functionality, my PR adds documentation in the following wiki page:

No documentation added

Commits

My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Code Quality

My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

junkaixue · 2023-09-14T04:35:26Z

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

+    while (--i >= 0) {
+      if (EntryMode.TTL.equals(mode)) {
+        createWithTTL(nodePaths.get(i), data, ttl);
+      } else {
+        create(nodePaths.get(i), data, i == 0 ? mode : parentMode);
+      }
+    }
+  }


This require try catch that if two threads parallel creation the children at the same time. Also I would imagine this should be same path as previous one.

@xyuanlu could you please help @GrantPSpencer walk through the idea?

I'm not sure I fully understand this race condition yet. I'll sync up with @xyuanlu to get more info

Changed to ignore when NodeExistsException thrown when we try to create the parents, only throw when we try to create the full node path. This means we don't throw an error if another thread has already created the parents we are trying to create.

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java

junkaixue · 2023-09-25T22:28:44Z

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

+      }
+    }
+
+    // Reattempt creation of children that failed due to NoNodeException


Update this comment.

junkaixue · 2023-09-25T22:30:44Z

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

+    }
+
+    // Reattempt creation of children that failed due to NoNodeException
+    while (--i >= 0) {


is this i ++ or --I? We should start with 0, right?

My approach is to use the same counter that we used in the previous loop. I'll walk through it below:
Lets say customer calls recursiveCreate("/first/second/third")
ZK only has "/first" node

This gets split into three separate nodes comprising the chain -->
nodePaths = ["/first/second/third", "/first/second", "/first"]

The first while loop we start with i=0 and increment up (i++) if we don't create it

i=0, Check if parent of nodePaths.get(i) -> "/first/second/third" exists, it does not so i++

i=1, Check if parent of nodePaths.get(i) -> "/first/second" exists, it exists so create "/first/second" and exit loop

The second while loop then goes back through the list of nodes and attempts to create the ones we missed

pre-decrement i, so i=0, create nodePaths.get(i)->"/first/second/third"

This approach of starting from the final child node is my attempt at optimizing over starting from the root node and then working down to the final child node. Unfortunately, my code has bloated quite a bite as a result :/

Also, we could have this as one loop, but I separated it into two as I did not want a possibility where a thread is stuck in an endless loop of incrementing/decrementing due to some unexpected behavior.

junkaixue · 2023-09-25T22:34:12Z

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java

+          if (i != 0) {
+            throw e;
+          }


It makes the API complicated behavior. I would suggest to consider if there is exist nodes, then we just stop disregard it is root race condition or not.

Otherwise, the API behavior would be a little bit complicate. We can just let user to retry create it recursively if it is root race condition.

Metaclient's normal create behavior is to throw error when creation fails due to NodeExistsException. So should we likewise just let this error be thrown and not catch it? For context, this change was to address your comments here regarding two threads creating children at same time:
#2607 (comment)

GrantPSpencer · 2023-09-29T00:12:56Z

Pull request approved by @junkaixue
Commit message: Add recursiveCreate functionality to metaclient

Co-authored-by: Grant Palau Spencer <[email protected]>

* MetaClientCache Part 1 - API's, configs, and builders (#2612) MetaClientCache Part 1 - API's, configs, and builders --------- Co-authored-by: mapeng <[email protected]> * Skip one time listener re-register for exists for ZkClient - MetaClient usage. (#2637) * Lattice cache - caching just data implementation (#2619) Lattice cache - caching just data implementation --------- Co-authored-by: mapeng <[email protected]> * Add recursiveCreate functionality to metaclient (#2607) Co-authored-by: Grant Palau Spencer <[email protected]> * Lattice Children Cache Implementation(#2623) Co-authored-by: mapeng <[email protected]> --------- Co-authored-by: Marcos Rico Peng <[email protected]> Co-authored-by: mapeng <[email protected]> Co-authored-by: Grant Paláu Spencer <[email protected]> Co-authored-by: Grant Palau Spencer <[email protected]>

GrantPSpencer changed the title ~~Metaclient~~ Add method to create full key path if it does not exist Aug 23, 2023

xyuanlu reviewed Aug 23, 2023

View reviewed changes

himanshukandwal reviewed Aug 24, 2023

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java Show resolved Hide resolved

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java Outdated Show resolved Hide resolved

asfgit force-pushed the metaclient branch from 29337ef to ddd6882 Compare August 28, 2023 20:58

GrantPSpencer added 9 commits August 29, 2023 10:22

add function to recursively create path if parent does not exist

d367a49

added createFullPath to stress test

8d3d97b

remove unused imports

fb5fd3b

Prevent ephemeral node parents

26ecec7

Address comments - recursion helper, excessive logging, naming

f062816

Add comments, add to unittests, and clean up recursion

5b011b9

formatting/typo

85cd958

fix test name

a120556

remove uncessary print statement

17ac48b

GrantPSpencer force-pushed the metaclient branch from 3593218 to 17ac48b Compare August 29, 2023 17:26

junkaixue reviewed Aug 30, 2023

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java Outdated Show resolved Hide resolved

GrantPSpencer added 2 commits August 31, 2023 15:56

Change to iterative create

4b034e1

another way to iterate over paths

652408e

junkaixue reviewed Sep 7, 2023

View reviewed changes

GrantPSpencer added 2 commits September 11, 2023 14:07

respond to reviewer comments

556f1b4

typo

3f989ab

junkaixue reviewed Sep 14, 2023

View reviewed changes

GrantPSpencer added 2 commits September 19, 2023 17:16

respond to reviewer feedback

866fc3e

catch create parent race condition exception

d2c4492

junkaixue approved these changes Sep 25, 2023

View reviewed changes

GrantPSpencer added 2 commits September 27, 2023 12:04

fix typo

abf081e

fix logic error in throwing nodeexists for final child node

b131572

github-actions bot added the CheckedAndApproved label Sep 29, 2023

xyuanlu merged commit 113b992 into apache:metaclient Oct 4, 2023
5 checks passed

asfgit pushed a commit that referenced this pull request Oct 5, 2023

Add recursiveCreate functionality to metaclient (#2607)

6bd97a9

Co-authored-by: Grant Palau Spencer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to create full key path if it does not exist #2607

Add method to create full key path if it does not exist #2607

GrantPSpencer commented Aug 23, 2023 •

edited by xyuanlu

Loading

junkaixue Sep 14, 2023

GrantPSpencer Sep 20, 2023

GrantPSpencer Sep 25, 2023

junkaixue Sep 25, 2023

junkaixue Sep 25, 2023

GrantPSpencer Sep 25, 2023 •

edited

Loading

junkaixue Sep 25, 2023

GrantPSpencer Sep 25, 2023

GrantPSpencer commented Sep 29, 2023

Add method to create full key path if it does not exist #2607

Add method to create full key path if it does not exist #2607

Conversation

GrantPSpencer commented Aug 23, 2023 • edited by xyuanlu Loading

Issues

Description

Tests

Changes that Break Backward Compatibility (Optional)

Documentation (Optional)

Commits

Code Quality

junkaixue Sep 14, 2023

Choose a reason for hiding this comment

GrantPSpencer Sep 20, 2023

Choose a reason for hiding this comment

GrantPSpencer Sep 25, 2023

Choose a reason for hiding this comment

junkaixue Sep 25, 2023

Choose a reason for hiding this comment

junkaixue Sep 25, 2023

Choose a reason for hiding this comment

GrantPSpencer Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

junkaixue Sep 25, 2023

Choose a reason for hiding this comment

GrantPSpencer Sep 25, 2023

Choose a reason for hiding this comment

GrantPSpencer commented Sep 29, 2023

GrantPSpencer commented Aug 23, 2023 •

edited by xyuanlu

Loading

GrantPSpencer Sep 25, 2023 •

edited

Loading