-
Notifications
You must be signed in to change notification settings - Fork 228
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.
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
Metaclient updater retry logic #2805
Metaclient updater retry logic #2805
Conversation
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java
Outdated
Show resolved
Hide resolved
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java
Outdated
Show resolved
Hide resolved
11bf562
|
T newData = updater.update(null); | ||
if (newData != null) { | ||
try { | ||
create(key, newData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the PR!
Question about the retry logic here.
For example, when user try to update node "/a/b/c", and there is no node "/a/b", set in line 228 with throw NoNodeException, and we reach line 243. Since there is no node "/a/b", create will also throw NoNodeException, and we keep retry until expire. Should we do recursive create instead? Or we just let it expire and forward the NoNodeException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good question. I am leaning towards not recursively creating, because that is the same behavior as helix zk client. However, if this API's contract with customers is "give me a node path, and I will make sure that it has the data you want" then I think there's a good argument for making our create attempt recursive rather than just on the leaf node. cc @junkaixue do you have thoughts on this?
And to clarify, this is the current flow if you tried to update path /a/b/c when no nodes in that path exist yet. (It should fail before retrying update)
try to update /a/b/c --> MetaClientNoNodeException is caught
try to create /a/b/ --> MetaClientNoNodeException this is not caught and so immediately fails
we do not retry update as method failed due to above exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also thinking of no creation. If this is update, it supposed to be updating something was there. It has the possibility that the node just be deleted recursively before update. Then update should be an invalidate operation and fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would second no creation as well. Say if we try to update "/a/b/c" and get no node exception, we just fail immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with @xyuanlu offline. Added createIfAbsent parameter. Base functionality is to not create the node, but will attempt to create leaf node if it is not present. It will not recursively create and will fail on first create() call if parent path does not exist.
Added relevant test cases to cover this
cc @junkaixue
https://github.com/apache/helix/actions/runs/9392174978
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Pull request approved by @xyuanlu , @junkaixue |
Issues
Retry logic for metaclient updater
Description
Metaclient updater currently does not retry on no node or version mismatch. This change includes logic to attempt to create mode if it does not exist and also retry on version mismatch.
Please take a look at the test case as well, is there a better way to recreate the version mismatch race condition?
Tests
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java #testUpdate
Changes that Break Backward Compatibility (Optional)
N/A
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)