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
Fix IndexShardRoutingTable's shard randomization to not throw out-of-bounds exceptions. #5561
Conversation
…bounds exceptions. Close elastic#5559
final List<Object> rotated = CollectionUtils.rotate(list, size); | ||
assertEquals(rotated.size(), list.size()); | ||
assertEquals(Iterables.size(rotated), list.size()); | ||
assertEquals(new HashSet<>(rotated), new HashSet<>(list)); |
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.
hmm I gues we should also test that the contents are the same here?
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.
grr nevermind I didn't see the last line pfff...
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 think that I think we should test is that the roation here is actually stable. It if you iterate the same list twice it has the same order?
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.
do you mean checking that rotating twice the same list gives the same output?
assertEquals(CollectionUtils.rotate(list, size), CollectionUtils.rotate(list, size));
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.
yeah
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.
or N times
this looks great - I left some commetns |
@s1monw I pushed a new commit |
LGTM |
Thanks Simon. |
Change elastic#5561 introduced a potential bug in that iterations that are performed on a thread are might not be visible to other threads due to the removal of the `volatile` keyword. Close elastic#6039
Change elastic#5561 introduced a potential bug in that iterations that are performed on a thread are might not be visible to other threads due to the removal of the `volatile` keyword. Close elastic#6039
Change elastic#5561 introduced a potential bug in that iterations that are performed on a thread are might not be visible to other threads due to the removal of the `volatile` keyword. Close elastic#6039
I initially wanted to make the diff minimal but this ended up being quite complicated
so I finally refactored a bit the way shards are randomized. Yet, it uses the same logic:
Close #5559