Skip to content

Commit 229d89d

Browse files
original-brownbearoriginal-brownbear
authored
Remove GroupShardsIterator and replace it with plain List (#116891)
There is no point in having `GroupShardsIterator`, it's mostly an unnecessary layer of indirection as it has no state and a single field only. It's only value could be seen in it hiding the ability to mutate the list it wraps, but that hardly justifies the overhead on the search path and extra code complexity. Moreover, the list it references is not copied/immutable in any way, so the value of hiding is limited also.
1 parent c1deef4 commit 229d89d

File tree

40 files changed

+159
-436
lines changed

40 files changed

+159
-436
lines changed

‎server/src/internalClusterTest/java/org/elasticsearch/index/shard/RemoveCorruptedShardDataCommandIT.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import org.elasticsearch.cluster.ClusterState;
3333
import org.elasticsearch.cluster.node.DiscoveryNode;
3434
import org.elasticsearch.cluster.node.DiscoveryNodes;
35-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
3635
import org.elasticsearch.cluster.routing.ShardIterator;
3736
import org.elasticsearch.cluster.routing.ShardRouting;
3837
import org.elasticsearch.cluster.routing.ShardRoutingState;
@@ -531,8 +530,7 @@ public void testResolvePath() throws Exception {
531530
nodeNameToNodeId.put(cursor.getValue().getName(), cursor.getKey());
532531
}
533532

534-
final GroupShardsIterator<ShardIterator> shardIterators = state.getRoutingTable()
535-
.activePrimaryShardsGrouped(new String[] { indexName }, false);
533+
final List<ShardIterator> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(new String[] { indexName }, false);
536534
final List<ShardIterator> iterators = iterableAsArrayList(shardIterators);
537535
final ShardRouting shardRouting = iterators.iterator().next().nextOrNull();
538536
assertThat(shardRouting, notNullValue());
@@ -571,8 +569,7 @@ public void testResolvePath() throws Exception {
571569

572570
private Path getPathToShardData(String indexName, String dirSuffix) {
573571
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
574-
GroupShardsIterator<ShardIterator> shardIterators = state.getRoutingTable()
575-
.activePrimaryShardsGrouped(new String[] { indexName }, false);
572+
List<ShardIterator> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(new String[] { indexName }, false);
576573
List<ShardIterator> iterators = iterableAsArrayList(shardIterators);
577574
ShardIterator shardIterator = RandomPicks.randomFrom(random(), iterators);
578575
ShardRouting shardRouting = shardIterator.nextOrNull();

‎server/src/internalClusterTest/java/org/elasticsearch/index/store/CorruptedFileIT.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
import org.elasticsearch.cluster.health.ClusterHealthStatus;
3535
import org.elasticsearch.cluster.metadata.IndexMetadata;
3636
import org.elasticsearch.cluster.node.DiscoveryNode;
37-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
3837
import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
3938
import org.elasticsearch.cluster.routing.ShardIterator;
4039
import org.elasticsearch.cluster.routing.ShardRouting;
@@ -311,8 +310,7 @@ public void testCorruptPrimaryNoReplica() throws ExecutionException, Interrupted
311310
}
312311
assertThat(response.getStatus(), is(ClusterHealthStatus.RED));
313312
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
314-
GroupShardsIterator<ShardIterator> shardIterators = state.getRoutingTable()
315-
.activePrimaryShardsGrouped(new String[] { "test" }, false);
313+
List<ShardIterator> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(new String[] { "test" }, false);
316314
for (ShardIterator iterator : shardIterators) {
317315
ShardRouting routing;
318316
while ((routing = iterator.nextOrNull()) != null) {
@@ -667,7 +665,7 @@ public void testReplicaCorruption() throws Exception {
667665

668666
private int numShards(String... index) {
669667
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
670-
GroupShardsIterator<?> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(index, false);
668+
List<?> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(index, false);
671669
return shardIterators.size();
672670
}
673671

@@ -695,8 +693,7 @@ private ShardRouting corruptRandomPrimaryFile() throws IOException {
695693
private ShardRouting corruptRandomPrimaryFile(final boolean includePerCommitFiles) throws IOException {
696694
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
697695
Index test = state.metadata().index("test").getIndex();
698-
GroupShardsIterator<ShardIterator> shardIterators = state.getRoutingTable()
699-
.activePrimaryShardsGrouped(new String[] { "test" }, false);
696+
List<ShardIterator> shardIterators = state.getRoutingTable().activePrimaryShardsGrouped(new String[] { "test" }, false);
700697
List<ShardIterator> iterators = iterableAsArrayList(shardIterators);
701698
ShardIterator shardIterator = RandomPicks.randomFrom(random(), iterators);
702699
ShardRouting shardRouting = shardIterator.nextOrNull();

‎server/src/internalClusterTest/java/org/elasticsearch/index/suggest/stats/SuggestStatsIT.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
1515
import org.elasticsearch.action.search.SearchRequestBuilder;
1616
import org.elasticsearch.cluster.ClusterState;
17-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
1817
import org.elasticsearch.cluster.routing.ShardIterator;
1918
import org.elasticsearch.cluster.routing.ShardRouting;
2019
import org.elasticsearch.index.search.stats.SearchStats;
@@ -24,6 +23,7 @@
2423
import org.elasticsearch.test.ESIntegTestCase;
2524

2625
import java.util.HashSet;
26+
import java.util.List;
2727
import java.util.Set;
2828

2929
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
@@ -146,7 +146,7 @@ private SearchRequestBuilder addSuggestions(SearchRequestBuilder request, int i)
146146

147147
private Set<String> nodeIdsWithIndex(String... indices) {
148148
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
149-
GroupShardsIterator<ShardIterator> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
149+
List<ShardIterator> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
150150
Set<String> nodes = new HashSet<>();
151151
for (ShardIterator shardIterator : allAssignedShardsGrouped) {
152152
for (ShardRouting routing : shardIterator) {
@@ -161,7 +161,7 @@ private Set<String> nodeIdsWithIndex(String... indices) {
161161

162162
protected int numAssignedShards(String... indices) {
163163
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
164-
GroupShardsIterator<?> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
164+
List<?> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
165165
return allAssignedShardsGrouped.size();
166166
}
167167
}

‎server/src/internalClusterTest/java/org/elasticsearch/search/stats/SearchStatsIT.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
1515
import org.elasticsearch.cluster.ClusterState;
1616
import org.elasticsearch.cluster.metadata.IndexMetadata;
17-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
1817
import org.elasticsearch.cluster.routing.ShardIterator;
1918
import org.elasticsearch.cluster.routing.ShardRouting;
2019
import org.elasticsearch.common.settings.Settings;
@@ -165,7 +164,7 @@ public void testSimpleStats() throws Exception {
165164

166165
private Set<String> nodeIdsWithIndex(String... indices) {
167166
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
168-
GroupShardsIterator<ShardIterator> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
167+
List<ShardIterator> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
169168
Set<String> nodes = new HashSet<>();
170169
for (ShardIterator shardIterator : allAssignedShardsGrouped) {
171170
for (ShardRouting routing : shardIterator) {
@@ -248,7 +247,7 @@ public void testOpenContexts() {
248247

249248
protected int numAssignedShards(String... indices) {
250249
ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
251-
GroupShardsIterator<?> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
250+
List<?> allAssignedShardsGrouped = state.routingTable().allAssignedShardsGrouped(indices, true);
252251
return allAssignedShardsGrouped.size();
253252
}
254253

‎server/src/main/java/org/elasticsearch/action/admin/cluster/shards/TransportClusterSearchShardsAction.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
2020
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
2121
import org.elasticsearch.cluster.node.DiscoveryNode;
22-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
2322
import org.elasticsearch.cluster.routing.ShardIterator;
2423
import org.elasticsearch.cluster.routing.ShardRouting;
2524
import org.elasticsearch.cluster.service.ClusterService;
@@ -34,6 +33,7 @@
3433

3534
import java.util.HashMap;
3635
import java.util.HashSet;
36+
import java.util.List;
3737
import java.util.Map;
3838
import java.util.Set;
3939

@@ -101,7 +101,7 @@ protected void masterOperation(
101101
}
102102

103103
Set<String> nodeIds = new HashSet<>();
104-
GroupShardsIterator<ShardIterator> groupShardsIterator = clusterService.operationRouting()
104+
List<ShardIterator> groupShardsIterator = clusterService.operationRouting()
105105
.searchShards(clusterState, concreteIndices, routingMap, request.preference());
106106
ShardRouting shard;
107107
ClusterSearchShardsGroup[] groupResponses = new ClusterSearchShardsGroup[groupShardsIterator.size()];

‎server/src/main/java/org/elasticsearch/action/admin/indices/diskusage/TransportAnalyzeIndexDiskUsageAction.java‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import org.elasticsearch.cluster.block.ClusterBlockLevel;
2222
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
2323
import org.elasticsearch.cluster.node.DiscoveryNode;
24-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
2524
import org.elasticsearch.cluster.routing.ShardIterator;
2625
import org.elasticsearch.cluster.routing.ShardRouting;
2726
import org.elasticsearch.cluster.service.ClusterService;
@@ -214,13 +213,8 @@ protected AnalyzeIndexDiskUsageResponse newResponse(
214213
}
215214

216215
@Override
217-
protected GroupShardsIterator<ShardIterator> shards(
218-
ClusterState clusterState,
219-
AnalyzeIndexDiskUsageRequest request,
220-
String[] concreteIndices
221-
) {
222-
final GroupShardsIterator<ShardIterator> groups = clusterService.operationRouting()
223-
.searchShards(clusterState, concreteIndices, null, null);
216+
protected List<ShardIterator> shards(ClusterState clusterState, AnalyzeIndexDiskUsageRequest request, String[] concreteIndices) {
217+
final List<ShardIterator> groups = clusterService.operationRouting().searchShards(clusterState, concreteIndices, null, null);
224218
for (ShardIterator group : groups) {
225219
// fails fast if any non-active groups
226220
if (group.size() == 0) {

‎server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import org.elasticsearch.cluster.block.ClusterBlockLevel;
2323
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
2424
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
25-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
2625
import org.elasticsearch.cluster.routing.ShardIterator;
2726
import org.elasticsearch.cluster.routing.ShardRouting;
2827
import org.elasticsearch.cluster.service.ClusterService;
@@ -145,7 +144,7 @@ protected ShardValidateQueryResponse readShardResponse(StreamInput in) throws IO
145144
}
146145

147146
@Override
148-
protected GroupShardsIterator<ShardIterator> shards(ClusterState clusterState, ValidateQueryRequest request, String[] concreteIndices) {
147+
protected List<ShardIterator> shards(ClusterState clusterState, ValidateQueryRequest request, String[] concreteIndices) {
149148
final String routing;
150149
if (request.allShards()) {
151150
routing = null;

‎server/src/main/java/org/elasticsearch/action/fieldcaps/RequestDispatcher.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import org.elasticsearch.action.OriginalIndices;
1919
import org.elasticsearch.cluster.ClusterState;
2020
import org.elasticsearch.cluster.node.DiscoveryNode;
21-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
2221
import org.elasticsearch.cluster.routing.ShardIterator;
2322
import org.elasticsearch.cluster.routing.ShardRouting;
2423
import org.elasticsearch.cluster.service.ClusterService;
@@ -93,7 +92,7 @@ final class RequestDispatcher {
9392
this.onComplete = new RunOnce(onComplete);
9493
this.indexSelectors = ConcurrentCollections.newConcurrentMap();
9594
for (String index : indices) {
96-
final GroupShardsIterator<ShardIterator> shardIts;
95+
final List<ShardIterator> shardIts;
9796
try {
9897
shardIts = clusterService.operationRouting().searchShards(clusterState, new String[] { index }, null, null);
9998
} catch (Exception e) {
@@ -250,7 +249,7 @@ private static class IndexSelector {
250249
private final Set<ShardId> unmatchedShardIds = new HashSet<>();
251250
private final Map<ShardId, Exception> failures = new HashMap<>();
252251

253-
IndexSelector(GroupShardsIterator<ShardIterator> shardIts) {
252+
IndexSelector(List<ShardIterator> shardIts) {
254253
for (ShardIterator shardIt : shardIts) {
255254
for (ShardRouting shard : shardIt) {
256255
nodeToShards.computeIfAbsent(shard.currentNodeId(), node -> new ArrayList<>()).add(shard);

‎server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import org.elasticsearch.action.support.SubscribableListener;
2323
import org.elasticsearch.action.support.TransportActions;
2424
import org.elasticsearch.cluster.ClusterState;
25-
import org.elasticsearch.cluster.routing.GroupShardsIterator;
2625
import org.elasticsearch.common.bytes.BytesReference;
2726
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
2827
import org.elasticsearch.common.util.Maps;
@@ -60,9 +59,9 @@
6059
import static org.elasticsearch.core.Strings.format;
6160

6261
/**
63-
* This is an abstract base class that encapsulates the logic to fan out to all shards in provided {@link GroupShardsIterator}
62+
* This is an abstract base class that encapsulates the logic to fan out to all shards in provided {@link List<SearchShardIterator>}
6463
* and collect the results. If a shard request returns a failure this class handles the advance to the next replica of the shard until
65-
* the shards replica iterator is exhausted. Each shard is referenced by position in the {@link GroupShardsIterator} which is later
64+
* the shards replica iterator is exhausted. Each shard is referenced by position in the {@link List<SearchShardIterator>} which is later
6665
* referred to as the {@code shardIndex}.
6766
* The fan out and collect algorithm is traditionally used as the initial phase which can either be a query execution or collection of
6867
* distributed frequencies
@@ -93,8 +92,8 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
9392
private final SearchTimeProvider timeProvider;
9493
private final SearchResponse.Clusters clusters;
9594

96-
protected final GroupShardsIterator<SearchShardIterator> toSkipShardsIts;
97-
protected final GroupShardsIterator<SearchShardIterator> shardsIts;
95+
protected final List<SearchShardIterator> toSkipShardsIts;
96+
protected final List<SearchShardIterator> shardsIts;
9897
private final SearchShardIterator[] shardIterators;
9998
private final AtomicInteger outstandingShards;
10099
private final int maxConcurrentRequestsPerNode;
@@ -116,7 +115,7 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
116115
Executor executor,
117116
SearchRequest request,
118117
ActionListener<SearchResponse> listener,
119-
GroupShardsIterator<SearchShardIterator> shardsIts,
118+
List<SearchShardIterator> shardsIts,
120119
SearchTimeProvider timeProvider,
121120
ClusterState clusterState,
122121
SearchTask task,
@@ -135,8 +134,8 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
135134
iterators.add(iterator);
136135
}
137136
}
138-
this.toSkipShardsIts = new GroupShardsIterator<>(toSkipIterators);
139-
this.shardsIts = new GroupShardsIterator<>(iterators);
137+
this.toSkipShardsIts = toSkipIterators;
138+
this.shardsIts = iterators;
140139
outstandingShards = new AtomicInteger(shardsIts.size());
141140
this.shardIterators = iterators.toArray(new SearchShardIterator[0]);
142141
// we later compute the shard index based on the natural order of the shards
@@ -171,8 +170,8 @@ protected void notifyListShards(
171170
SearchSourceBuilder sourceBuilder
172171
) {
173172
progressListener.notifyListShards(
174-
SearchProgressListener.buildSearchShards(this.shardsIts),
175-
SearchProgressListener.buildSearchShards(toSkipShardsIts),
173+
SearchProgressListener.buildSearchShardsFromIter(this.shardsIts),
174+
SearchProgressListener.buildSearchShardsFromIter(toSkipShardsIts),
176175
clusters,
177176
sourceBuilder == null || sourceBuilder.size() > 0,
178177
timeProvider

0 commit comments

Comments
 (0)