Skip to content

Commit 86c9a5d

Browse files
benchaplinbenchaplin
committed
Rework change to ignore blocked indices before shard resolution
1 parent 76ecade commit 86c9a5d

File tree

8 files changed

+63
-194
lines changed

8 files changed

+63
-194
lines changed

‎server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
package org.elasticsearch.search;
1111

12+
import com.carrotsearch.randomizedtesting.annotations.Repeat;
13+
1214
import org.elasticsearch.action.index.IndexRequestBuilder;
1315
import org.elasticsearch.action.search.ClosePointInTimeRequest;
1416
import org.elasticsearch.action.search.OpenPointInTimeRequest;
@@ -37,6 +39,7 @@
3739
import static org.elasticsearch.test.ClusterServiceUtils.setState;
3840
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
3941

42+
@Repeat(iterations = 100)
4043
public class SearchWithIndexBlocksIT extends ESIntegTestCase {
4144

4245
public void testSearchIndicesWithIndexRefreshBlocks() {

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,6 @@ private void runCoordinatorRewritePhase() {
186186
assert assertSearchCoordinationThread();
187187
final List<SearchShardIterator> matchedShardLevelRequests = new ArrayList<>();
188188
for (SearchShardIterator searchShardIterator : shardsIts) {
189-
if (searchShardIterator.prefiltered() == false && searchShardIterator.skip()) {
190-
// This implies the iterator was skipped due to an index level block,
191-
// not a remote can-match run.
192-
continue;
193-
}
194-
195189
final CanMatchNodeRequest canMatchNodeRequest = new CanMatchNodeRequest(
196190
request,
197191
searchShardIterator.getOriginalIndices().indicesOptions(),

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

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,28 +41,19 @@ public final class SearchShardIterator implements Comparable<SearchShardIterator
4141

4242
/**
4343
* Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards
44-
* for a given <code>shardId</code>.
44+
* this the given <code>shardId</code>.
4545
*
4646
* @param clusterAlias the alias of the cluster where the shard is located
4747
* @param shardId shard id of the group
4848
* @param shards shards to iterate
4949
* @param originalIndices the indices that the search request originally related to (before any rewriting happened)
50-
* @param skip if true, then this group won't have matches (due to an index level block),
51-
* and it can be safely skipped from the search
5250
*/
53-
public SearchShardIterator(
54-
@Nullable String clusterAlias,
55-
ShardId shardId,
56-
List<ShardRouting> shards,
57-
OriginalIndices originalIndices,
58-
boolean skip
59-
) {
60-
this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, skip);
51+
public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List<ShardRouting> shards, OriginalIndices originalIndices) {
52+
this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, false);
6153
}
6254

6355
/**
6456
* Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards
65-
* for a given <code>shardId</code>.
6657
*
6758
* @param clusterAlias the alias of the cluster where the shard is located
6859
* @param shardId shard id of the group
@@ -71,8 +62,7 @@ public SearchShardIterator(
7162
* @param searchContextId the point-in-time specified for this group if exists
7263
* @param searchContextKeepAlive the time interval that data nodes should extend the keep alive of the point-in-time
7364
* @param prefiltered if true, then this group already executed the can_match phase
74-
* @param skip if true, then this group won't have matches (due to can match, or an index level block),
75-
* and it can be safely skipped from the search
65+
* @param skip if true, then this group won't have matches, and it can be safely skipped from the search
7666
*/
7767
public SearchShardIterator(
7868
@Nullable String clusterAlias,
@@ -93,6 +83,7 @@ public SearchShardIterator(
9383
assert searchContextKeepAlive == null || searchContextId != null;
9484
this.prefiltered = prefiltered;
9585
this.skip = skip;
86+
assert skip == false || prefiltered : "only prefiltered shards are skip-able";
9687
}
9788

9889
/**

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

Lines changed: 20 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,6 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest,
148148
Property.NodeScope
149149
);
150150

151-
// Marker to indicate this index's shards should be skipped in a search
152-
private static final OriginalIndices SKIPPED_INDICES = new OriginalIndices(Strings.EMPTY_ARRAY, IndicesOptions.strictExpandOpen());
153-
154151
private final ThreadPool threadPool;
155152
private final ClusterService clusterService;
156153
private final TransportService transportService;
@@ -237,10 +234,6 @@ private Map<String, OriginalIndices> buildPerIndexOriginalIndices(
237234
for (String index : indices) {
238235
if (hasBlocks) {
239236
blocks.indexBlockedRaiseException(projectId, ClusterBlockLevel.READ, index);
240-
if (blocks.hasIndexBlock(projectState.projectId(), index, IndexMetadata.INDEX_REFRESH_BLOCK)) {
241-
res.put(index, SKIPPED_INDICES);
242-
continue;
243-
}
244237
}
245238

246239
String[] aliases = indexNameExpressionResolver.allIndexAliases(projectState.metadata(), index, indicesAndAliases);
@@ -596,7 +589,7 @@ public void onFailure(Exception e) {}
596589
);
597590
}
598591

599-
static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShardsToSearch) {
592+
static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShards) {
600593
// if there's a kNN search, always use DFS_QUERY_THEN_FETCH
601594
if (searchRequest.hasKnnSearch()) {
602595
searchRequest.searchType(DFS_QUERY_THEN_FETCH);
@@ -611,7 +604,7 @@ static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShard
611604
}
612605

613606
// optimize search type for cases where there is only one shard group to search on
614-
if (oneOrZeroShardsToSearch) {
607+
if (oneOrZeroShards) {
615608
// if we only have one group, then we always want Q_T_F, no need for DFS, and no need to do THEN since we hit one shard
616609
searchRequest.searchType(QUERY_THEN_FETCH);
617610
}
@@ -1312,7 +1305,7 @@ private void executeSearch(
13121305

13131306
Map<String, Float> concreteIndexBoosts = resolveIndexBoosts(searchRequest, projectState.cluster());
13141307

1315-
adjustSearchType(searchRequest, oneOrZeroShardsToSearch(shardIterators));
1308+
adjustSearchType(searchRequest, shardIterators.size() <= 1);
13161309

13171310
final DiscoveryNodes nodes = projectState.cluster().nodes();
13181311
BiFunction<String, String, Transport.Connection> connectionLookup = buildConnectionLookup(
@@ -1345,33 +1338,6 @@ private void executeSearch(
13451338
);
13461339
}
13471340

1348-
/**
1349-
* Determines if only one (or zero) search shard iterators will be searched.
1350-
* (At this point, iterators may be marked as skipped due to index level blockers).
1351-
* We expect skipped iterators to be unlikely, so returning fast after we see more
1352-
* than one "not skipped" is an intended optimization.
1353-
*
1354-
* @param searchShardIterators all the shard iterators derived from indices being searched
1355-
* @return true if there are no more than one shard iterators, or if there are no more than
1356-
* one not marked to skip
1357-
*/
1358-
private boolean oneOrZeroShardsToSearch(List<SearchShardIterator> searchShardIterators) {
1359-
if (searchShardIterators.size() <= 1) {
1360-
return true;
1361-
}
1362-
1363-
int notSkippedCount = 0;
1364-
for (SearchShardIterator searchShardIterator : searchShardIterators) {
1365-
if (searchShardIterator.skip() == false) {
1366-
notSkippedCount++;
1367-
if (notSkippedCount > 1) {
1368-
return false;
1369-
}
1370-
}
1371-
}
1372-
return true;
1373-
}
1374-
13751341
Executor asyncSearchExecutor(final String[] indices) {
13761342
boolean seenSystem = false;
13771343
boolean seenCritical = false;
@@ -1898,6 +1864,7 @@ List<SearchShardIterator> getLocalShardsIterator(
18981864
Set<ResolvedExpression> indicesAndAliases,
18991865
String[] concreteIndices
19001866
) {
1867+
concreteIndices = ignoreBlockedIndices(projectState, concreteIndices);
19011868
var routingMap = indexNameExpressionResolver.resolveSearchRouting(
19021869
projectState.metadata(),
19031870
searchRequest.routing(),
@@ -1924,18 +1891,27 @@ List<SearchShardIterator> getLocalShardsIterator(
19241891
final ShardId shardId = shardRouting.shardId();
19251892
OriginalIndices finalIndices = originalIndices.get(shardId.getIndex().getName());
19261893
assert finalIndices != null;
1927-
list[i++] = new SearchShardIterator(
1928-
clusterAlias,
1929-
shardId,
1930-
shardRouting.getShardRoutings(),
1931-
finalIndices,
1932-
finalIndices == SKIPPED_INDICES
1933-
);
1894+
list[i++] = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices);
19341895
}
19351896
// the returned list must support in-place sorting, so this is the most memory efficient we can do here
19361897
return Arrays.asList(list);
19371898
}
19381899

1900+
static String[] ignoreBlockedIndices(ProjectState projectState, String[] concreteIndices) {
1901+
// optimization: mostly we do not have any blocks so there's no point in the expensive per-index checking
1902+
boolean hasIndexBlocks = projectState.blocks().indices(projectState.projectId()).isEmpty() == false;
1903+
if (hasIndexBlocks) {
1904+
logger.info("Has index block: {}", projectState.blocks().toString());
1905+
return Arrays.stream(concreteIndices)
1906+
.filter(
1907+
index -> projectState.blocks()
1908+
.hasIndexBlock(projectState.projectId(), index, IndexMetadata.INDEX_REFRESH_BLOCK) == false
1909+
)
1910+
.toArray(String[]::new);
1911+
}
1912+
return concreteIndices;
1913+
}
1914+
19391915
private interface TelemetryListener {
19401916
void setRemotes(int count);
19411917

‎server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ private AbstractSearchAsyncAction<SearchPhaseResult> createAction(
8282
null,
8383
request,
8484
listener,
85-
Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null, false)),
85+
Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null)),
8686
timeProvider,
8787
ClusterState.EMPTY_STATE,
8888
null,
@@ -153,8 +153,7 @@ public void testBuildShardSearchTransportRequest() {
153153
clusterAlias,
154154
new ShardId(new Index("name", "foo"), 1),
155155
Collections.emptyList(),
156-
new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand()),
157-
false
156+
new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand())
158157
);
159158
ShardSearchRequest shardSearchTransportRequest = action.buildShardSearchRequest(iterator, 10);
160159
assertEquals(IndicesOptions.strictExpand(), shardSearchTransportRequest.indicesOptions());

‎server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -640,8 +640,7 @@ public void testSkipUnavailableSearchShards() throws InterruptedException {
640640
null,
641641
new ShardId(index, 0),
642642
Collections.emptyList(),
643-
originalIndices,
644-
false
643+
originalIndices
645644
);
646645
// Skip all the shards
647646
searchShardIterator.skip(true);
@@ -761,7 +760,7 @@ static List<SearchShardIterator> getShardsIter(
761760
}
762761
Collections.shuffle(started, random());
763762
started.addAll(initializing);
764-
list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices, false));
763+
list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices));
765764
}
766765
return list;
767766
}

‎server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java‎

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,7 @@ private static List<ShardRouting> randomShardRoutings(ShardId shardId, int numRe
4545

4646
public void testShardId() {
4747
ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomInt());
48-
SearchShardIterator searchShardIterator = new SearchShardIterator(
49-
null,
50-
shardId,
51-
Collections.emptyList(),
52-
OriginalIndices.NONE,
53-
false
54-
);
48+
SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), OriginalIndices.NONE);
5549
assertSame(shardId, searchShardIterator.shardId());
5650
}
5751

@@ -61,7 +55,7 @@ public void testGetOriginalIndices() {
6155
new String[] { randomAlphaOfLengthBetween(3, 10) },
6256
IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean())
6357
);
64-
SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices, false);
58+
SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices);
6559
assertSame(originalIndices, searchShardIterator.getOriginalIndices());
6660
}
6761

@@ -72,8 +66,7 @@ public void testGetClusterAlias() {
7266
clusterAlias,
7367
shardId,
7468
Collections.emptyList(),
75-
OriginalIndices.NONE,
76-
false
69+
OriginalIndices.NONE
7770
);
7871
assertEquals(clusterAlias, searchShardIterator.getClusterAlias());
7972
}
@@ -171,22 +164,15 @@ public void testCompareTo() {
171164
for (String uuid : uuids) {
172165
ShardId shardId = new ShardId(index, uuid, i);
173166
shardIterators.add(
174-
new SearchShardIterator(
175-
null,
176-
shardId,
177-
randomShardRoutings(shardId),
178-
OriginalIndicesTests.randomOriginalIndices(),
179-
false
180-
)
167+
new SearchShardIterator(null, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices())
181168
);
182169
for (String cluster : clusters) {
183170
shardIterators.add(
184171
new SearchShardIterator(
185172
cluster,
186173
shardId,
187174
randomShardRoutings(shardId),
188-
OriginalIndicesTests.randomOriginalIndices(),
189-
false
175+
OriginalIndicesTests.randomOriginalIndices()
190176
)
191177
);
192178
}
@@ -231,12 +217,6 @@ public void testCompareToEqualItems() {
231217
private static SearchShardIterator randomSearchShardIterator() {
232218
String clusterAlias = randomBoolean() ? null : randomAlphaOfLengthBetween(5, 10);
233219
ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomIntBetween(0, Integer.MAX_VALUE));
234-
return new SearchShardIterator(
235-
clusterAlias,
236-
shardId,
237-
randomShardRoutings(shardId),
238-
OriginalIndicesTests.randomOriginalIndices(),
239-
false
240-
);
220+
return new SearchShardIterator(clusterAlias, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices());
241221
}
242222
}

0 commit comments

Comments
 (0)