#2965 Atomic ZNode creation on node registration#2978
#2965 Atomic ZNode creation on node registration#2978jacob-netguardians wants to merge 4 commits intoapache:masterfrom
Conversation
junkaixue
left a comment
There was a problem hiding this comment.
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"
|
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. |
junkaixue
left a comment
There was a problem hiding this comment.
In general, a good practice is splitting PR into: 1) purely logic change, 2) purely reformat / refactoring without touch logic.
| "SwapOutInstance {} is {} + {} and SwapInInstance {} is OFFLINE + {} for cluster {}. Swap will" | ||
| + " not complete unless SwapInInstance instance is ONLINE.", | ||
| swapOutInstanceName, swapOutLiveInstance != null ? "ONLINE" : "OFFLINE", | ||
| + " not complete unless SwapInInstance instance is ONLINE.", swapOutInstanceName, |
There was a problem hiding this comment.
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.
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)