From 9e38125464a291526e32866f8de9b4fd5d572a8c Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 26 Feb 2020 13:58:20 +0100 Subject: [PATCH] Clarify when shard iterators get sorted (#52810) Currently we have two ways to create a GroupShardsIterator: one that will resort the iterators based on their natural ordering, and another one that will leave them in their original order. This is currently done through two constructors, one that accepts a single argument which does the sorting, and another which accepts a second boolean argument to control whether sorting should happen or not. This second constructor is only called externally to disable the sorting. By introducing a specific method to create a sorted shard iterator we clarify and make it easier to track when we do sort and when we do not as the iterators are externally sorted. --- .../search/AbstractSearchAsyncAction.java | 4 +- .../search/CanMatchPreFilterSearchPhase.java | 4 +- .../action/search/TransportSearchAction.java | 19 +++++-- .../cluster/routing/GroupShardsIterator.java | 14 ++--- .../cluster/routing/OperationRouting.java | 2 +- .../cluster/routing/RoutingTable.java | 4 +- .../routing/GroupShardsIteratorTests.java | 53 +++++++++++++------ 7 files changed, 66 insertions(+), 34 deletions(-) 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 22da82dfeb6..8caf615dc1d 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 aba32d2c850..59debedbcf8 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 @@ final class CanMatchPreFilterSearchPhase extends AbstractSearchAsyncAction(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 @@ final class CanMatchPreFilterSearchPhase extends AbstractSearchAsyncAction 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 74d3e783adb..43178fa5031 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 @@ public class TransportSearchAction extends HandledTransportAction(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 @@ public class TransportSearchAction extends HandledTransportAction { - 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 a9904c96d02..1cb105ac775 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 b13377687ad..e7481b06a9f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/OperationRouting.java @@ -136,7 +136,7 @@ public class OperationRouting { 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 67da9f2010f..c80d03ffe3e 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 @@ public class RoutingTable implements Iterable, Diffable(set); + return GroupShardsIterator.sortAndCreate(set); } public ShardsIterator allShards(String[] indices) { @@ -321,7 +321,7 @@ public class RoutingTable implements Iterable, Diffable(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 f7fe59e501b..45c57a0cdce 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 class GroupShardsIteratorTests extends ESTestCase { 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 class GroupShardsIteratorTests extends ESTestCase { } 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 class GroupShardsIteratorTests extends ESTestCase { 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 class GroupShardsIteratorTests extends ESTestCase { 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); } }