Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Oct 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d367a49
add function to recursively create path if parent does not exist
GrantPSpencer Aug 19, 2023
8d3d97b
added createFullPath to stress test
GrantPSpencer Aug 20, 2023
fb5fd3b
remove unused imports
GrantPSpencer Aug 20, 2023
26ecec7
Prevent ephemeral node parents
GrantPSpencer Aug 23, 2023
f062816
Address comments - recursion helper, excessive logging, naming
GrantPSpencer Aug 23, 2023
5b011b9
Add comments, add to unittests, and clean up recursion
GrantPSpencer Aug 24, 2023
85cd958
formatting/typo
GrantPSpencer Aug 24, 2023
a120556
fix test name
GrantPSpencer Aug 28, 2023
17ac48b
remove uncessary print statement
GrantPSpencer Aug 29, 2023
4b034e1
Change to iterative create
GrantPSpencer Aug 31, 2023
652408e
another way to iterate over paths
GrantPSpencer Aug 31, 2023
556f1b4
respond to reviewer comments
GrantPSpencer Sep 11, 2023
3f989ab
typo
GrantPSpencer Sep 11, 2023
866fc3e
respond to reviewer feedback
GrantPSpencer Sep 20, 2023
d2c4492
catch create parent race condition exception
GrantPSpencer Sep 25, 2023
abf081e
fix typo
GrantPSpencer Sep 27, 2023
b131572
fix logic error in throwing nodeexists for final child node
GrantPSpencer Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,21 @@ public Stat (EntryMode mode, int version, long ctime, long mtime, long etime) {
*/
void create(final String key, final T data, final EntryMode mode);

/**
* Create an entry of given EntryMode with given key and data. If any parent node in the node
* hierarchy does not exist, then the parent node will attempt to be created. The entry will not
* be created if there is an existing entry with the same full key. Ephemeral nodes cannot have
* children, so only the final child in the created path will be ephemeral.
*/
void recursiveCreate(final String key, final T Data, final EntryMode mode);

/**
* Create a TTL entry with given key, data, and expiry time (ttl). If any parent node in the node
* hierarchy does not exist, then the parent node will attempt to be created. The entry will not be created if
* there is an existing entry with the same full key.
*/
void recursiveCreateWithTTL(String key, T data, long ttl);

/**
* Create an entry of given EntryMode with given key, data, and expiry time (ttl).
* The entry will automatically purge when reached expiry time and has no children.
Expand Down
49 changes: 48 additions & 1 deletion meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil.separateIntoUniqueNodePaths;
import static org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil.translateZkExceptionToMetaclientException;


Expand Down Expand Up @@ -104,7 +105,7 @@ public void create(String key, Object data) {
}

@Override
public void create(String key, Object data, MetaClientInterface.EntryMode mode) {
public void create(String key, Object data, EntryMode mode) {

try {
_zkClient.create(key, data, ZkMetaClientUtil.convertMetaClientMode(mode));
Expand All @@ -115,6 +116,52 @@ public void create(String key, Object data, MetaClientInterface.EntryMode mode)
}
}

@Override
public void recursiveCreate(String key, T data, EntryMode mode) {
// Function named recursiveCreate to match naming scheme, but actual work is iterative
iterativeCreate(key, data, mode, -1);
}

@Override
public void recursiveCreateWithTTL(String key, T data, long ttl) {
iterativeCreate(key, data, EntryMode.TTL, ttl);
}

private void iterativeCreate(String key, T data, EntryMode mode, long ttl) {
List<String> nodePaths = separateIntoUniqueNodePaths(key);
int i = 0;
// Ephemeral nodes cant have children, so change mode when creating parents
EntryMode parentMode = (EntryMode.EPHEMERAL.equals(mode) ?
EntryMode.PERSISTENT : mode);
GrantPSpencer marked this conversation as resolved.
Show resolved Hide resolved

// Iterate over paths, starting with full key then attempting each successive parent
// Try /a/b/c, if fails due to NoNode then try /a/b .. etc..
while (i < nodePaths.size()) {
try {
if (EntryMode.TTL.equals(mode)) {
createWithTTL(nodePaths.get(i), data, ttl);
} else {
create(nodePaths.get(i), data, i == 0 ? mode : parentMode);
}
break;
// NoNodeException thrown when parent path does not exist. We allow this and re-attempt
// creation of these nodes below
} catch (MetaClientNoNodeException ignoredParentDoesntExistException) {
GrantPSpencer marked this conversation as resolved.
Show resolved Hide resolved
i++;
}

}

// Reattempt creation of children that failed due to NoNodeException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this comment.

while (--i >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@GrantPSpencer GrantPSpencer Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (EntryMode.TTL.equals(mode)) {
createWithTTL(nodePaths.get(i), data, ttl);
} else {
create(nodePaths.get(i), data, i == 0 ? mode : parentMode);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Override
public void createWithTTL(String key, T data, long ttl) {
try {
Expand Down
21 changes: 21 additions & 0 deletions meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,25 @@ public static MetaClientException.ReturnCode translateZooKeeperCodeToMetaClientC
return MetaClientException.ReturnCode.DB_USER_ERROR;
}
}

public static String getZkParentPath(String path) {
if (path.equals("/")) {
return null;
}

int idx = path.lastIndexOf('/');
return idx == 0 ? "/" : path.substring(0, idx);
GrantPSpencer marked this conversation as resolved.
Show resolved Hide resolved
}

// Splits a path into the paths for each node along the way.
// /a/b/c --> /a/b/c, /a/b, /a
public static List<String> separateIntoUniqueNodePaths(String path) {
junkaixue marked this conversation as resolved.
Show resolved Hide resolved
ArrayList<String> nodePaths = new ArrayList<>();
GrantPSpencer marked this conversation as resolved.
Show resolved Hide resolved
String tempStr = path;
while (tempStr.length() > 1) {
nodePaths.add(tempStr);
tempStr = getZkParentPath(tempStr);
}
return nodePaths;
}
}
30 changes: 30 additions & 0 deletions meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestStressZkClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.helix.metaclient.api.*;
import org.apache.helix.metaclient.datamodel.DataRecord;
import org.apache.helix.metaclient.exception.MetaClientException;
import org.apache.helix.metaclient.exception.MetaClientNodeExistsException;
import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
import org.apache.helix.metaclient.recipes.lock.DataRecordSerializer;
import org.testng.Assert;
Expand Down Expand Up @@ -129,6 +130,35 @@ public void testCreateAndRenewTTL() {
Assert.assertEquals(_zkMetaClient.countDirectChildren(zkParentKey), 0);
}

@Test
public void testRecursiveCreate() {
final String zkParentKey = "/stressZk_testRecursiveCreate";
_zkMetaClient.create(zkParentKey, ENTRY_STRING_VALUE);

int count = (int) Math.pow(TEST_ITERATION_COUNT, 1/3d);
for (int i = 0; i < count; i++) {

for (int j = 0; j < count; j++) {

for (int k = 0; k < count; k++) {
String key = zkParentKey + "/" + i + "/" + j + "/" + k;
_zkMetaClient.recursiveCreate(key, String.valueOf(k), PERSISTENT);
Assert.assertEquals(String.valueOf(k), _zkMetaClient.get(key));
}
}
try {
_zkMetaClient.recursiveCreate(zkParentKey + "/" + i, "should_fail", PERSISTENT);
Assert.fail("Should have failed due to node existing");
} catch (MetaClientNodeExistsException ignoredException) {
}
}

// cleanup
_zkMetaClient.recursiveDelete(zkParentKey);
Assert.assertEquals(_zkMetaClient.countDirectChildren(zkParentKey), 0);

}

@Test
public void testGet() {
final String zkParentKey = "/stressZk_testGet";
Expand Down
46 changes: 45 additions & 1 deletion meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.apache.helix.metaclient.exception.MetaClientException;
import org.apache.helix.metaclient.api.DirectChildChangeListener;

import java.io.StringWriter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
Expand All @@ -38,7 +40,6 @@
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.commons.lang3.NotImplementedException;
import org.apache.helix.metaclient.api.ConnectStateChangeListener;
import org.apache.helix.metaclient.api.DataChangeListener;
import org.apache.helix.metaclient.api.Op;
import org.apache.helix.metaclient.api.OpResult;
Expand All @@ -50,6 +51,7 @@

import static org.apache.helix.metaclient.api.DataChangeListener.ChangeType.ENTRY_UPDATE;
import static org.apache.helix.metaclient.api.MetaClientInterface.EntryMode.CONTAINER;
import static org.apache.helix.metaclient.api.MetaClientInterface.EntryMode.EPHEMERAL;
import static org.apache.helix.metaclient.api.MetaClientInterface.EntryMode.PERSISTENT;


Expand Down Expand Up @@ -97,6 +99,48 @@ public void testCreateTTL() {
}
}

@Test
public void testRecursiveCreate() {
final String path = "/Test/ZkMetaClient/_fullPath";


try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
zkMetaClient.connect();
MetaClientInterface.EntryMode mode = EPHEMERAL;

// Should succeed even if one of the parent nodes exists
String extendedPath = "/A" + path;
zkMetaClient.create("/A", ENTRY_STRING_VALUE, PERSISTENT);
zkMetaClient.recursiveCreate(extendedPath, ENTRY_STRING_VALUE, mode);
Assert.assertNotNull(zkMetaClient.exists(extendedPath));

// Should succeed if no parent nodes exist
zkMetaClient.recursiveCreate(path, ENTRY_STRING_VALUE, mode);
Assert.assertNotNull(zkMetaClient.exists(path));
Assert.assertEquals(zkMetaClient.getDataAndStat("/Test").getRight().getEntryType(), PERSISTENT);
Assert.assertEquals(zkMetaClient.getDataAndStat(path).getRight().getEntryType(), mode);

// Should throw NodeExistsException if child node exists
zkMetaClient.recursiveCreate(path, ENTRY_STRING_VALUE, mode);
Assert.fail("Should have failed due to node already created");
} catch (MetaClientException e) {
Assert.assertEquals(e.getMessage(), "org.apache.helix.zookeeper.zkclient.exception.ZkNodeExistsException: org.apache.zookeeper.KeeperException$NodeExistsException: KeeperErrorCode = NodeExists for /Test/ZkMetaClient/_fullPath");
System.out.println(e.getMessage());
}

}

@Test
public void testRecursiveCreateWithTTL() {
final String path = "/Test/ZkMetaClient/_fullPath/withTTL";

try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
zkMetaClient.connect();
zkMetaClient.recursiveCreateWithTTL(path, ENTRY_STRING_VALUE, 1000);
Assert.assertNotNull(zkMetaClient.exists(path));
}
}

@Test
public void testRenewTTL() {
final String key = "/TestZkMetaClient_testRenewTTL_1";
Expand Down