Simplify usages of shardsWithState#92055
Conversation
idegtiarenko
added
>non-issue
:Distributed/Allocation
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
Looks good, assuming CI is happy. Since this is only test changes, can we backport to 8.6 too?
| } | ||
| if (elements instanceof Collection) { | ||
| return new ArrayList<>((Collection) elements); | ||
| return new ArrayList<>((Collection<E>) elements); |
There was a problem hiding this comment.
Prevent raw usage of parameterized class
|
Pinging @elastic/es-distributed (Team:Distributed) |
| ? iterableAsArrayList(routingNodes.unassigned()) | ||
| : routingNodes.stream() | ||
| .flatMap(routingNode -> routingNode.shardsWithState(state).stream()) | ||
| .collect(toCollection(ArrayList::new)); |
There was a problem hiding this comment.
Some tests modify the output of this method (eg shuffle)
There was a problem hiding this comment.
I think I'd rather have those tests copy the result into a new list instead and make this immutable. It's only a couple of spots to change I think.
| } | ||
| } | ||
| return shards; | ||
| public static List<ShardRouting> shardsWithState(RoutingNodes routingNodes, String index, ShardRoutingState states) { |
There was a problem hiding this comment.
None of the callers of this method need the mutability AFAICT?
| ? iterableAsArrayList(routingNodes.unassigned()) | ||
| : routingNodes.stream() | ||
| .flatMap(routingNode -> routingNode.shardsWithState(state).stream()) | ||
| .collect(toCollection(ArrayList::new)); |
There was a problem hiding this comment.
I think I'd rather have those tests copy the result into a new list instead and make this immutable. It's only a couple of spots to change I think.
| List<ShardRouting> startedShards = RoutingNodesHelper.shardsWithState(state.getRoutingNodes(), ShardRoutingState.STARTED); | ||
| Collections.shuffle(startedShards, random()); | ||
| return state.nodes().get(startedShards.get(0).currentNodeId()).getName(); | ||
| return state.nodes().get(randomFrom(startedShards).currentNodeId()).getName(); |
There was a problem hiding this comment.
No need to shuffle entire collection to get a single random element.
| ShardRoutingState.INITIALIZING | ||
| ); | ||
| initializingShards.sort(Comparator.comparing(ShardRouting::shardId).thenComparing(ShardRouting::primary, Boolean::compare)); | ||
| List<ShardRouting> shards = randomSubsetOf(Math.min(randomIntBetween(1, 100), initializingShards.size()), initializingShards); |
There was a problem hiding this comment.
I think this sorting is unnecessary. randomSubsetOf shuffles the collection internally.
3 participants
This is a followup to #91991