Fixed the searchRing function for consistentHash #2460
Conversation
|
Attaching two pair graphs to show that this change really makes sense. Here are results of 'TestFadeIn_consistent-hash,_4', the first one before fix, the second if after. This test introduces three "old" nodes and one "fading in" one. As we can see, fix makes load between three "old" nodes more even. |
|
Something similar in 'TestFadeIn/consistent-hash,_5'. Three "old" nodes, three "fading in" ones. Before fix, one of three "old" nodes get less load, after fix all three get the same load. |
|
CI failed because of #2457 (comment) |
|
@RomanZavodskikh Please update commit message (and PR description) with the description of the problem and why this is the proper fix. |
It is not obvious how to interpret the graphs tbh. We should think how to make it easier to eyeball sanity check, maybe color-code datapoints according to endpoint. |
|
@RomanZavodskikh Would it be possible to craft a specific test that fails before the fix and passes after? |
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
from
656267f
to
24e762b
Compare
|
@AlexanderYastrebov FYI:
|
|
Great work! |
|
👍 |
|
Nice find, makes sense! |
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
from
24e762b
to
d905bad
Compare
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
2 times, most recently
from
187043d
to
696fdf5
Compare
Signed-off-by: Roman Zavodskikh <[email protected]>
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
from
696fdf5
to
c74f478
Compare
|
By the way, I've added the legend. Now figures look like this.
|
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
2 times, most recently
from
2c72a3d
to
8f94d9e
Compare
Signed-off-by: Roman Zavodskikh <[email protected]>
This fix is needed to comply the requirements of input for binary search https://pkg.go.dev/sort#Search Search uses binary search to find and return the smallest index i in [0, n) at which f(i) is true, assuming that on the range [0, n), f(i) == true implies f(i+1) == true. That is, Search requires that f is false for some (possibly empty) prefix of the input range [0, n) and then true for the (possibly empty) remainder Signed-off-by: Roman Zavodskikh <[email protected]>
Signed-off-by: Roman Zavodskikh <[email protected]>
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
from
8f94d9e
to
bb2126f
Compare
Signed-off-by: Roman Zavodskikh <[email protected]>
RomanZavodskikh
force-pushed
the
fixSearchRing
branch
from
bb2126f
to
af3d546
Compare
|
👍 |
1 similar comment
|
👍 |
This fix is needed to comply the requirements of input for binary search https://pkg.go.dev/sort#Search
This requirement is not satisfied before this PR, because
skipEndpoint(idx int)may return false for every specific index regardless of other indicies. So, even if skipEndpoint(i) is true, nothing can stop skipEndpoint(i+1) being false.