Make sure ConsistentHashingNodeProvider returns unique candidates#17228
Make sure ConsistentHashingNodeProvider returns unique candidates#17228rongrong merged 1 commit intoprestodb:masterfrom
Conversation
rschlussel
left a comment
There was a problem hiding this comment.
I see there's no release note. what was the motivation for this change? Does it fix a bug or other issue?
There was a problem hiding this comment.
Instead of building the node list here, why not add to the unique set no matter what, and then convert the set to an immutable list before returning it?
There was a problem hiding this comment.
ordering is important here.
Welcome back! Yes, it's fixing a bug. The method should return |
There was a problem hiding this comment.
Could this be an infinite loop? e.g., when num of workers is < count? Add a test for this case?
It is also possible that this will take a lot of iterations to find next unique candidate if the hash function happens to return a visited virtual node for a lot of times.
How about searching it in the candidates map to find the next unique candidate instead of relying on a random function? Specifically, if we find a duplicate node, we get its iterator/pointer in the map and check the following nodes according to their order in the map until we find a new node or have exhausted all node.
There was a problem hiding this comment.
Hmm, I think the assumption here is that count is small. 2 is used right now. But I agree we should add a check. I think in any realistic scenario the probability of continuously hitting the same virtual nodes is pretty low but that's also assuming count is small. Next physical node on the ring seems better.
1e3defd to
22f894e
Compare
|
@rschlussel @beinan Can you guys take a look? Thanks! |
...ain/src/main/java/com/facebook/presto/execution/scheduler/ConsistentHashingNodeProvider.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/execution/scheduler/ConsistentHashingNodeProvider.java
Outdated
Show resolved
Hide resolved
000d433 to
f425728
Compare
f425728 to
4c0557e
Compare
Test plan - unit test