Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

@jpountz
Copy link
Contributor

commented Mar 26, 2014

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:

  • rotations to shuffle shards,
  • an AtomicInteger to generate the distances to use for the rotations.

Close #5559

…bounds

exceptions.

Close #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));

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

hmm I gues we should also test that the contents are the same here?

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

grr nevermind I didn't see the last line pfff...

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

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?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 27, 2014

Author Contributor

do you mean checking that rotating twice the same list gives the same output?

assertEquals(CollectionUtils.rotate(list, size), CollectionUtils.rotate(list, size));

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

yeah

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

or N times

return rotate1(list, d);
}

private static <T> List<T> rotate1(final List<T> list, final int distance) {

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

hmm can't we just make this a static inner class instead of haveing a second rotate1 method?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 27, 2014

Author Contributor

good point

/**
* A shuffler for shards whose primary goal is to balance load.
*/
public interface ShardShuffler {

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

not sure if we need an interface for this but I guess it doesn't hurt either. I wonder if we can make it an abstract class and overload shuffle to only take a list?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 27, 2014

Author Contributor

I'm not sure to understand, what default implementation would you provide?

private final int limit;

private volatile int counter;
private ListIterator<ShardRouting> iterator;

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

I like this class man that is so much cleaner

}
}

// TODO: we can move to random based on ThreadLocalRandom, or make it pluggable

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

"BOOM!"


private final AtomicInteger seed;

public RotationShardShuffler() {

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

shoudl this class allow to work without ThreadLocalRandom? it just take the seed?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 27, 2014

Author Contributor

I'm not sure to understand what you are suggesting, but I didn't want to change the behavior at all, so since it was previously using an AtomicInteger initialized based on a random integer, I kept it.

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 27, 2014

Contributor

yeah I was just asking if we should pass the seed in the ctor and call ThreadLocalRandom where thsi is used?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 27, 2014

Author Contributor

Ah ok. Will do.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2014

this looks great - I left some commetns

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2014

@s1monw I pushed a new commit

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2014

LGTM

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2014

Thanks Simon.

@jpountz jpountz closed this Mar 27, 2014
@jpountz jpountz deleted the jpountz:fix/IndexShardRoutingTable branch Mar 27, 2014
jpountz added a commit to jpountz/elasticsearch that referenced this pull request May 5, 2014
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
jpountz added a commit that referenced this pull request May 5, 2014
Change #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 #6039
jpountz added a commit that referenced this pull request May 5, 2014
Change #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 #6039
jpountz added a commit that referenced this pull request May 5, 2014
Change #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 #6039
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
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
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.