diff --git a/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java b/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java index 22da82dfeb681..8caf615dc1d07 100644 --- a/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java @@ -116,8 +116,8 @@ abstract class AbstractSearchAsyncAction exten iterators.add(iterator); } } - this.toSkipShardsIts = new GroupShardsIterator<>(toSkipIterators, false); - this.shardsIts = new GroupShardsIterator<>(iterators, false); + this.toSkipShardsIts = new GroupShardsIterator<>(toSkipIterators); + this.shardsIts = new GroupShardsIterator<>(iterators); // we need to add 1 for non active partition, since we count it in the total. This means for each shard in the iterator we sum up // it's number of active shards but use 1 as the default if no replica of a shard is active at this point. // on a per shards level we use shardIt.remaining() to increment the totalOps pointer but add 1 for the current shard result diff --git a/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java b/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java index aba32d2c850a0..59debedbcf8d0 100644 --- a/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java @@ -113,7 +113,7 @@ private GroupShardsIterator getIterator(CanMatchSearchPhase return shardsIts; } FieldSortBuilder fieldSort = FieldSortBuilder.getPrimaryFieldSortOrNull(source); - return new GroupShardsIterator<>(sortShards(shardsIts, results.minAndMaxes, fieldSort.order()), false); + return new GroupShardsIterator<>(sortShards(shardsIts, results.minAndMaxes, fieldSort.order())); } private static List sortShards(GroupShardsIterator shardsIts, @@ -122,7 +122,7 @@ private static List sortShards(GroupShardsIterator shardsIts.get(ord)) + .map(shardsIts::get) .collect(Collectors.toList()); } diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java index 24f0e04371588..2f7e8e338ea24 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -553,10 +553,10 @@ static GroupShardsIterator mergeShardsIterators(GroupShards for (ShardIterator shardIterator : localShardsIterator) { shards.add(new SearchShardIterator(localClusterAlias, shardIterator.shardId(), shardIterator.getShardRoutings(), localIndices)); } - return new GroupShardsIterator<>(shards); + return GroupShardsIterator.sortAndCreate(shards); } - private AbstractSearchAsyncAction searchAsyncAction(SearchTask task, SearchRequest searchRequest, + private AbstractSearchAsyncAction searchAsyncAction(SearchTask task, SearchRequest searchRequest, GroupShardsIterator shardIterators, SearchTimeProvider timeProvider, BiFunction connectionLookup, @@ -572,8 +572,19 @@ private AbstractSearchAsyncAction searchAsyncAction(SearchTask task, SearchReque return new CanMatchPreFilterSearchPhase(logger, searchTransportService, connectionLookup, aliasFilter, concreteIndexBoosts, indexRoutings, executor, searchRequest, listener, shardIterators, timeProvider, clusterStateVersion, task, (iter) -> { - AbstractSearchAsyncAction action = searchAsyncAction(task, searchRequest, iter, timeProvider, connectionLookup, - clusterStateVersion, aliasFilter, concreteIndexBoosts, indexRoutings, listener, false, clusters); + AbstractSearchAsyncAction action = searchAsyncAction( + task, + searchRequest, + iter, + timeProvider, + connectionLookup, + clusterStateVersion, + aliasFilter, + concreteIndexBoosts, + indexRoutings, + listener, + false, + clusters); return new SearchPhase(action.getName()) { @Override public void run() { diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/GroupShardsIterator.java b/server/src/main/java/org/elasticsearch/cluster/routing/GroupShardsIterator.java index a9904c96d020f..1cb105ac775e3 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/GroupShardsIterator.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/GroupShardsIterator.java @@ -35,19 +35,19 @@ public final class GroupShardsIterator implements private final List iterators; /** - * Constructs a enw GroupShardsIterator from the given list. + * Constructs a new sorted GroupShardsIterator from the given list. Items are sorted based on their natural ordering. + * @see PlainShardIterator#compareTo(ShardIterator) + * @see org.elasticsearch.action.search.SearchShardIterator#compareTo(ShardIterator) */ - public GroupShardsIterator(List iterators) { - this(iterators, true); + public static GroupShardsIterator sortAndCreate(List iterators) { + CollectionUtil.timSort(iterators); + return new GroupShardsIterator<>(iterators); } /** * Constructs a new GroupShardsIterator from the given list. */ - public GroupShardsIterator(List iterators, boolean useSort) { - if (useSort) { - CollectionUtil.timSort(iterators); - } + public GroupShardsIterator(List iterators) { this.iterators = iterators; } diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java b/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java index 6d9397db3b377..bc5d51b918796 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java @@ -95,7 +95,7 @@ public GroupShardsIterator searchShards(ClusterState clusterState set.add(iterator); } } - return new GroupShardsIterator<>(new ArrayList<>(set)); + return GroupShardsIterator.sortAndCreate(new ArrayList<>(set)); } private static final Map> EMPTY_ROUTING = Collections.emptyMap(); diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java index b595efb3a5b4b..f54e31292d6aa 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java @@ -260,7 +260,7 @@ private GroupShardsIterator allSatisfyingPredicateShardsGrouped(S } } } - return new GroupShardsIterator<>(set); + return GroupShardsIterator.sortAndCreate(set); } public ShardsIterator allShards(String[] indices) { @@ -321,7 +321,7 @@ public GroupShardsIterator activePrimaryShardsGrouped(String[] in } } } - return new GroupShardsIterator<>(set); + return GroupShardsIterator.sortAndCreate(set); } @Override diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/GroupShardsIteratorTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/GroupShardsIteratorTests.java index f7fe59e501b33..45c57a0cdce84 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/GroupShardsIteratorTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/GroupShardsIteratorTests.java @@ -69,7 +69,7 @@ public void testSize() { ShardId shardId = new ShardId(index, 1); list.add(new PlainShardIterator(shardId, randomShardRoutings(shardId, 0))); } - GroupShardsIterator iter = new GroupShardsIterator<>(list); + GroupShardsIterator iter = new GroupShardsIterator<>(list); assertEquals(7, iter.totalSizeWith1ForEmpty()); assertEquals(5, iter.size()); assertEquals(6, iter.totalSize()); @@ -106,13 +106,24 @@ public void testIterate() { } Collections.shuffle(list, random()); - List actualIterators = new ArrayList<>(); - GroupShardsIterator iter = new GroupShardsIterator<>(list); - for (ShardIterator shardsIterator : iter) { - actualIterators.add(shardsIterator); + { + GroupShardsIterator unsorted = new GroupShardsIterator<>(list); + GroupShardsIterator iter = new GroupShardsIterator<>(list); + List actualIterators = new ArrayList<>(); + for (ShardIterator shardsIterator : iter) { + actualIterators.add(shardsIterator); + } + assertEquals(actualIterators, list); + } + { + GroupShardsIterator iter = GroupShardsIterator.sortAndCreate(list); + List actualIterators = new ArrayList<>(); + for (ShardIterator shardsIterator : iter) { + actualIterators.add(shardsIterator); + } + CollectionUtil.timSort(actualIterators); + assertEquals(actualIterators, list); } - CollectionUtil.timSort(actualIterators); - assertEquals(actualIterators, list); } public void testOrderingWithSearchShardIterators() { @@ -123,7 +134,7 @@ public void testOrderingWithSearchShardIterators() { String[] clusters = generateRandomStringArray(5, 10, false, false); Arrays.sort(clusters); - List expected = new ArrayList<>(); + List sorted = new ArrayList<>(); int numShards = randomIntBetween(1, 10); for (int i = 0; i < numShards; i++) { for (String index : indices) { @@ -131,23 +142,33 @@ public void testOrderingWithSearchShardIterators() { ShardId shardId = new ShardId(index, uuid, i); SearchShardIterator shardIterator = new SearchShardIterator(null, shardId, GroupShardsIteratorTests.randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()); - expected.add(shardIterator); + sorted.add(shardIterator); for (String cluster : clusters) { SearchShardIterator remoteIterator = new SearchShardIterator(cluster, shardId, GroupShardsIteratorTests.randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()); - expected.add(remoteIterator); + sorted.add(remoteIterator); } } } } - List shuffled = new ArrayList<>(expected); + List shuffled = new ArrayList<>(sorted); Collections.shuffle(shuffled, random()); - List actualIterators = new ArrayList<>(); - GroupShardsIterator iter = new GroupShardsIterator<>(shuffled); - for (SearchShardIterator searchShardIterator : iter) { - actualIterators.add(searchShardIterator); + { + List actualIterators = new ArrayList<>(); + GroupShardsIterator iter = new GroupShardsIterator<>(shuffled); + for (SearchShardIterator searchShardIterator : iter) { + actualIterators.add(searchShardIterator); + } + assertEquals(shuffled, actualIterators); + } + { + List actualIterators = new ArrayList<>(); + GroupShardsIterator iter = GroupShardsIterator.sortAndCreate(shuffled); + for (SearchShardIterator searchShardIterator : iter) { + actualIterators.add(searchShardIterator); + } + assertEquals(sorted, actualIterators); } - assertEquals(expected, actualIterators); } }