-
Notifications
You must be signed in to change notification settings - Fork 244
#2965 Atomic ZNode creation on node registration#2978
#2965 Atomic ZNode creation on node registration #2978jacob-netguardians wants to merge 4 commits intoapache:master from
Conversation
@junkaixue
junkaixue
left a comment
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.
One thing is the helix format.
Second, please add test. You can just leave an existing node there to see whether this failure is something expected behavior.
Reformatted changed file according to the "helix format"
jacob-netguardians
commented
Dec 9, 2024
Formatted the code in "helix format". It did not only touch the lines I had modified.
For the test, I am sorry, I do not know how to reproduce the situation in a controlled way in a test.
The problem appeared from time to time in my integration tests, and I already spent a few hours tracking it down to this.
@junkaixue
junkaixue
left a comment
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.
In general, a good practice is splitting PR into: 1) purely logic change, 2) purely reformat / refactoring without touch logic.
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.
Suggest not to do entire file reformat, hard for reviewing if it is format change or logical change.
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.
Should be better now.
This reverts commit 43ba0b0.
Reformatted changed file according to the "helix format"
Issues
Fixes #2965
Description
If two nodes try to register at about the same time in the cluster, there may be a race condition while effectively adding the instances to the cluster description in zookeeper (the second one seeing an incomplete node hierarchy and later on refusing to accept tasks). Having an atomic creation of those nodes in zookeeper should prevent that.
Tests
No additional tests.
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)