From 2aa74e3a45e0c96436fbfe70b6db051c15671768 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 4 Jun 2025 19:00:20 -0400 Subject: [PATCH 01/18] Skip indices that have an index refresh block --- .../action/search/TransportSearchAction.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 8d704853a5f8e..4fb07f64f9a4c 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -148,6 +148,8 @@ public class TransportSearchAction extends HandledTransportAction buildPerIndexOriginalIndices( for (String index : indices) { if (hasBlocks) { blocks.indexBlockedRaiseException(projectState.projectId(), ClusterBlockLevel.READ, index); + if (blocks.hasIndexBlock(projectState.projectId(), index, IndexMetadata.INDEX_REFRESH_BLOCK)) { + res.put(index, SKIPPED_INDICES); + continue; + } } String[] aliases = indexNameExpressionResolver.allIndexAliases(projectState.metadata(), index, indicesAndAliases); @@ -1889,7 +1895,9 @@ List getLocalShardsIterator( final ShardId shardId = shardRouting.shardId(); OriginalIndices finalIndices = originalIndices.get(shardId.getIndex().getName()); assert finalIndices != null; - list[i++] = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices); + var it = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices); + it.skip(finalIndices == SKIPPED_INDICES); + list[i++] = it; } // the returned list must support in-place sorting, so this is the most memory efficient we can do here return Arrays.asList(list); From 9c705cd17ed0d49c7d98029e138d7d9e97b701f8 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Mon, 9 Jun 2025 00:00:14 -0400 Subject: [PATCH 02/18] Construct the iterator skipped --- .../action/search/SearchShardIterator.java | 17 ++++++++++++++++- .../action/search/TransportSearchAction.java | 4 +--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 00ff8f33f5659..57b7ab2ded031 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -25,6 +25,7 @@ * Iterator for shards used in the search api, which also holds the {@link OriginalIndices} * of the search request (useful especially with cross-cluster search, as each cluster has its own set of original indices) as well as * the cluster alias. + * * @see OriginalIndices */ public final class SearchShardIterator implements Comparable { @@ -52,6 +53,21 @@ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List< this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, false); } + /** + * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards + * this the a given shardId. + * + * @param clusterAlias the alias of the cluster where the shard is located + * @param shardId shard id of the group + * @param shards shards to iterate + * @param originalIndices the indices that the search request originally related to (before any rewriting happened) + * @param skip if true, then this group won't ha + * ve matches, and it can be safely skipped from the search + */ + public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List shards, OriginalIndices originalIndices, boolean skip) { + this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, skip); + } + /** * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards * @@ -83,7 +99,6 @@ public SearchShardIterator( assert searchContextKeepAlive == null || searchContextId != null; this.prefiltered = prefiltered; this.skip = skip; - assert skip == false || prefiltered : "only prefiltered shards are skip-able"; } /** 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 4fb07f64f9a4c..72ea941c748b6 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -1895,9 +1895,7 @@ List getLocalShardsIterator( final ShardId shardId = shardRouting.shardId(); OriginalIndices finalIndices = originalIndices.get(shardId.getIndex().getName()); assert finalIndices != null; - var it = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices); - it.skip(finalIndices == SKIPPED_INDICES); - list[i++] = it; + list[i++] = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices, finalIndices == SKIPPED_INDICES); } // the returned list must support in-place sorting, so this is the most memory efficient we can do here return Arrays.asList(list); From 1c757210573f92078c286ae0758d9fa50b882be4 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Mon, 9 Jun 2025 00:10:24 -0400 Subject: [PATCH 03/18] Fix javadocs --- .../elasticsearch/action/search/SearchShardIterator.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 57b7ab2ded031..8c1037345702b 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -25,7 +25,6 @@ * Iterator for shards used in the search api, which also holds the {@link OriginalIndices} * of the search request (useful especially with cross-cluster search, as each cluster has its own set of original indices) as well as * the cluster alias. - * * @see OriginalIndices */ public final class SearchShardIterator implements Comparable { @@ -42,7 +41,7 @@ public final class SearchShardIterator implements ComparableshardId. + * for a given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group @@ -55,14 +54,13 @@ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List< /** * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards - * this the a given shardId. + * for a given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group * @param shards shards to iterate * @param originalIndices the indices that the search request originally related to (before any rewriting happened) - * @param skip if true, then this group won't ha - * ve matches, and it can be safely skipped from the search + * @param skip if true, then this group won't have matches, and it can be safely skipped from the search */ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List shards, OriginalIndices originalIndices, boolean skip) { this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, skip); @@ -70,6 +68,7 @@ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List< /** * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards + * for a given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group From 1ecc4476eda46c1672deac3cad046265d16c61b8 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Mon, 9 Jun 2025 01:26:48 -0400 Subject: [PATCH 04/18] Add unit test --- .../search/TransportSearchActionTests.java | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index 3c5dc6b39292c..b42440955eb8d 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -41,6 +41,8 @@ import org.elasticsearch.cluster.node.VersionInformation; import org.elasticsearch.cluster.project.TestProjectResolvers; import org.elasticsearch.cluster.routing.IndexRoutingTable; +import org.elasticsearch.cluster.routing.OperationRouting; +import org.elasticsearch.cluster.routing.ShardIterator; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.TestShardRouting; @@ -133,6 +135,8 @@ import static org.hamcrest.Matchers.hasSize; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -1812,4 +1816,106 @@ public void onFailure(Exception ex) { assertTrue(ESTestCase.terminate(threadPool)); } } + + public void testSkippedIndicesWithRefreshBlock() { + final ProjectId projectId = randomProjectIdOrDefault(); + + String normalIndexName = "test-normal"; + String blockedIndexName = "test-blocked"; + final String[] indexNames = {normalIndexName, blockedIndexName}; + final Index normalIndex = new Index(normalIndexName, UUIDs.randomBase64UUID()); + final Index blockedIndex = new Index(blockedIndexName, UUIDs.randomBase64UUID()); + final int numberOfShards = randomIntBetween(1, 3); + final int numberOfReplicas = randomIntBetween(0, 1); + final int totalShards = numberOfShards + numberOfShards * numberOfReplicas; + + ClusterState clusterState = ClusterStateCreationUtils.stateWithAssignedPrimariesAndReplicas( + projectId, + indexNames, + numberOfShards, + numberOfReplicas + ); + ClusterBlocks.Builder blocksBuilder = ClusterBlocks.builder().blocks(clusterState.blocks()); + blocksBuilder.addIndexBlock(projectId, "test-blocked", IndexMetadata.INDEX_REFRESH_BLOCK); + clusterState = ClusterState.builder(clusterState).blocks(blocksBuilder).build(); + List shardIts = new ArrayList<>(); + for (int i = 0; i < totalShards; i++) { + shardIts.add(new ShardIterator(new ShardId(normalIndex, randomInt()), Collections.emptyList())); + shardIts.add(new ShardIterator(new ShardId(blockedIndex, randomInt()), Collections.emptyList())); + } + final OperationRouting operationRouting = mock(OperationRouting.class); + when( + operationRouting.searchShards( + eq(clusterState.projectState(projectId)), + eq(indexNames), + any(), + nullable(String.class), + any(), + any() + ) + ).thenReturn(shardIts); + ClusterService clusterService = mock(ClusterService.class); + when(clusterService.state()).thenReturn(clusterState); + when(clusterService.getSettings()).thenReturn(Settings.EMPTY); + when(clusterService.operationRouting()).thenReturn(operationRouting); + + Settings settings = Settings.builder() + .put("node.name", TransportSearchAction.class.getSimpleName()) + .build(); + TransportVersion transportVersion = TransportVersionUtils.getNextVersion(TransportVersions.MINIMUM_CCS_VERSION, true); + ThreadPool threadPool = new ThreadPool(settings, MeterRegistry.NOOP, new DefaultBuiltInExecutorBuilders()); + try { + TransportService transportService = MockTransportService.createNewService( + Settings.EMPTY, + VersionInformation.CURRENT, + transportVersion, + threadPool + ); + NodeClient client = new NodeClient(settings, threadPool); + SearchService searchService = mock(SearchService.class); + when(searchService.getRewriteContext(any(), any(), any(), anyBoolean())).thenReturn( + new QueryRewriteContext(null, null, null, null, null, null) + ); + + TransportSearchAction transportSearchAction = new TransportSearchAction( + threadPool, + new NoneCircuitBreakerService(), + transportService, + searchService, + null, + new SearchTransportService(transportService, client, null), + null, + clusterService, + new ActionFilters(Collections.emptySet()), + TestProjectResolvers.usingRequestHeader(threadPool.getThreadContext()), + TestIndexNameExpressionResolver.newInstance(threadPool.getThreadContext()), + null, + null, + new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry()), + client, + new UsageService() + ); + + SearchRequest searchRequest = new SearchRequest(indexNames); + searchRequest.allowPartialSearchResults(true); + List searchShardIts = transportSearchAction.getLocalShardsIterator( + clusterState.projectState(projectId), + searchRequest, + searchRequest.getLocalClusterAlias(), + new HashSet<>(), + indexNames + ); + + assertThat(searchShardIts.size(), equalTo(shardIts.size())); + for (SearchShardIterator searchShardIt : searchShardIts) { + if (searchShardIt.skip()) { + assertThat(searchShardIt.shardId().getIndexName(), equalTo("test-blocked")); + } else { + assertThat(searchShardIt.shardId().getIndexName(), equalTo("test-normal")); + } + } + } finally { + assertTrue(ESTestCase.terminate(threadPool)); + } + } } From cdb4bc11d90038e5b45d6cb1799c0b1bf49bb68b Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Mon, 9 Jun 2025 17:13:29 +0000 Subject: [PATCH 05/18] [CI] Auto commit changes from spotless --- .../elasticsearch/action/search/SearchShardIterator.java | 8 +++++++- .../action/search/TransportSearchAction.java | 8 +++++++- .../action/search/TransportSearchActionTests.java | 6 ++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 8c1037345702b..7b0b81a8d3182 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -62,7 +62,13 @@ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List< * @param originalIndices the indices that the search request originally related to (before any rewriting happened) * @param skip if true, then this group won't have matches, and it can be safely skipped from the search */ - public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List shards, OriginalIndices originalIndices, boolean skip) { + public SearchShardIterator( + @Nullable String clusterAlias, + ShardId shardId, + List shards, + OriginalIndices originalIndices, + boolean skip + ) { this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, skip); } 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 72ea941c748b6..bf7aa11ec550b 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -1895,7 +1895,13 @@ List getLocalShardsIterator( final ShardId shardId = shardRouting.shardId(); OriginalIndices finalIndices = originalIndices.get(shardId.getIndex().getName()); assert finalIndices != null; - list[i++] = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices, finalIndices == SKIPPED_INDICES); + list[i++] = new SearchShardIterator( + clusterAlias, + shardId, + shardRouting.getShardRoutings(), + finalIndices, + finalIndices == SKIPPED_INDICES + ); } // the returned list must support in-place sorting, so this is the most memory efficient we can do here return Arrays.asList(list); diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index b42440955eb8d..5cf5fc7f30eea 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -1822,7 +1822,7 @@ public void testSkippedIndicesWithRefreshBlock() { String normalIndexName = "test-normal"; String blockedIndexName = "test-blocked"; - final String[] indexNames = {normalIndexName, blockedIndexName}; + final String[] indexNames = { normalIndexName, blockedIndexName }; final Index normalIndex = new Index(normalIndexName, UUIDs.randomBase64UUID()); final Index blockedIndex = new Index(blockedIndexName, UUIDs.randomBase64UUID()); final int numberOfShards = randomIntBetween(1, 3); @@ -1859,9 +1859,7 @@ public void testSkippedIndicesWithRefreshBlock() { when(clusterService.getSettings()).thenReturn(Settings.EMPTY); when(clusterService.operationRouting()).thenReturn(operationRouting); - Settings settings = Settings.builder() - .put("node.name", TransportSearchAction.class.getSimpleName()) - .build(); + Settings settings = Settings.builder().put("node.name", TransportSearchAction.class.getSimpleName()).build(); TransportVersion transportVersion = TransportVersionUtils.getNextVersion(TransportVersions.MINIMUM_CCS_VERSION, true); ThreadPool threadPool = new ThreadPool(settings, MeterRegistry.NOOP, new DefaultBuiltInExecutorBuilders()); try { From 3f86fb8db5de4b0711047df8d83ba93a33ae3946 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 11 Jun 2025 12:52:50 -0400 Subject: [PATCH 06/18] Rewrite DFS if processing one or zero unskipped shard iterators --- .../action/search/TransportSearchAction.java | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) 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 bf7aa11ec550b..2a89faf8da31f 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -594,7 +594,7 @@ public void onFailure(Exception e) {} ); } - static void adjustSearchType(SearchRequest searchRequest, boolean singleShard) { + static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroValidShards) { // if there's a kNN search, always use DFS_QUERY_THEN_FETCH if (searchRequest.hasKnnSearch()) { searchRequest.searchType(DFS_QUERY_THEN_FETCH); @@ -609,7 +609,7 @@ static void adjustSearchType(SearchRequest searchRequest, boolean singleShard) { } // optimize search type for cases where there is only one shard group to search on - if (singleShard) { + if (oneOrZeroValidShards) { // 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 searchRequest.searchType(QUERY_THEN_FETCH); } @@ -1310,7 +1310,8 @@ private void executeSearch( Map concreteIndexBoosts = resolveIndexBoosts(searchRequest, projectState.cluster()); - adjustSearchType(searchRequest, shardIterators.size() == 1); + boolean oneOrZeroValidShards = shardIterators.size() == 1 || allOrAllButOneSkipped(shardIterators); + adjustSearchType(searchRequest, oneOrZeroValidShards); final DiscoveryNodes nodes = projectState.cluster().nodes(); BiFunction connectionLookup = buildConnectionLookup( @@ -1343,6 +1344,30 @@ private void executeSearch( ); } + /** + * Determines if all, or all but one, iterators are skipped. + * (At this point, iterators may be marked as skipped due to index level blockers). + * We expect skipped iteators to be unlikely, so returning fast after we see more + * than one "not skipped" is an intended optimization. + * + * @param searchShardIterators all the shard iterators derived from indices being searched + * @return true if all of them are already skipped, or only one is not skipped + */ + private boolean allOrAllButOneSkipped(List searchShardIterators) { + int notSkippedCount = 0; + + for (SearchShardIterator searchShardIterator : searchShardIterators) { + if (searchShardIterator.skip() == false) { + notSkippedCount++; + if (notSkippedCount > 1) { + return false; + } + } + } + + return true; + } + Executor asyncSearchExecutor(final String[] indices) { boolean seenSystem = false; boolean seenCritical = false; From 0edc27cf0b2e081908fff8e6c3a167148893061c Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 11 Jun 2025 13:32:36 -0400 Subject: [PATCH 07/18] Make can-match support already skipped shard iterators --- .../action/search/CanMatchPreFilterSearchPhase.java | 6 ++++++ .../elasticsearch/action/search/SearchShardIterator.java | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-) 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 e42f8127c5e97..8adb9180e3bae 100644 --- a/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java @@ -186,6 +186,12 @@ private void runCoordinatorRewritePhase() { assert assertSearchCoordinationThread(); final List matchedShardLevelRequests = new ArrayList<>(); for (SearchShardIterator searchShardIterator : shardsIts) { + if (searchShardIterator.prefiltered() == false && searchShardIterator.skip()) { + // This implies the iterator was skipped due to an index level block, + // not a remote can-match run. + continue; + } + final CanMatchNodeRequest canMatchNodeRequest = new CanMatchNodeRequest( request, searchShardIterator.getOriginalIndices().indicesOptions(), diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 7b0b81a8d3182..53d5e1f5b717f 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -60,7 +60,8 @@ public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List< * @param shardId shard id of the group * @param shards shards to iterate * @param originalIndices the indices that the search request originally related to (before any rewriting happened) - * @param skip if true, then this group won't have matches, and it can be safely skipped from the search + * @param skip if true, then this group won't have matches (due to an index level block), + * and it can be safely skipped from the search */ public SearchShardIterator( @Nullable String clusterAlias, @@ -83,7 +84,8 @@ public SearchShardIterator( * @param searchContextId the point-in-time specified for this group if exists * @param searchContextKeepAlive the time interval that data nodes should extend the keep alive of the point-in-time * @param prefiltered if true, then this group already executed the can_match phase - * @param skip if true, then this group won't have matches, and it can be safely skipped from the search + * @param skip if true, then this group won't have matches (due to can match, or an index level block), + * and it can be safely skipped from the search */ public SearchShardIterator( @Nullable String clusterAlias, From 9de6f060645c61253257cd203194b168f8f6041b Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 11 Jun 2025 15:41:06 -0400 Subject: [PATCH 08/18] Add IT for executing search and PIT against refresh blocked indices --- .../search/SearchWithIndexBlocksIT.java | 137 ++++++++++++++++++ .../cluster/metadata/IndexMetadata.java | 3 +- .../search/TransportSearchActionTests.java | 2 +- 3 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java new file mode 100644 index 0000000000000..af7a25e1d5dc2 --- /dev/null +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -0,0 +1,137 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.search; + +import org.elasticsearch.action.admin.indices.readonly.AddIndexBlockRequest; +import org.elasticsearch.action.admin.indices.readonly.TransportAddIndexBlockAction; +import org.elasticsearch.action.search.ClosePointInTimeRequest; +import org.elasticsearch.action.search.OpenPointInTimeRequest; +import org.elasticsearch.action.search.SearchRequest; +import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.action.search.TransportClosePointInTimeAction; +import org.elasticsearch.action.search.TransportOpenPointInTimeAction; +import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.core.TimeValue; +import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.search.builder.PointInTimeBuilder; +import org.elasticsearch.search.builder.SearchSourceBuilder; +import org.elasticsearch.test.ESIntegTestCase; + +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; + +public class SearchWithIndexBlocksIT extends ESIntegTestCase { + + public void testSearchIndexWithIndexRefreshBlock() { + createIndex("test"); + + var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); + client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); + + indexRandom( + true, + prepareIndex("test").setId("1").setSource("field", "value"), + prepareIndex("test").setId("2").setSource("field", "value"), + prepareIndex("test").setId("3").setSource("field", "value"), + prepareIndex("test").setId("4").setSource("field", "value"), + prepareIndex("test").setId("5").setSource("field", "value"), + prepareIndex("test").setId("6").setSource("field", "value") + ); + + assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 0); + } + + public void testSearchMultipleIndicesEachWithAnIndexRefreshBlock() { + createIndex("test"); + createIndex("test2"); + + var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test", "test2"); + client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); + + indexRandom( + true, + prepareIndex("test").setId("1").setSource("field", "value"), + prepareIndex("test").setId("2").setSource("field", "value"), + prepareIndex("test").setId("3").setSource("field", "value"), + prepareIndex("test").setId("4").setSource("field", "value"), + prepareIndex("test").setId("5").setSource("field", "value"), + prepareIndex("test").setId("6").setSource("field", "value"), + prepareIndex("test2").setId("1").setSource("field", "value"), + prepareIndex("test2").setId("2").setSource("field", "value"), + prepareIndex("test2").setId("3").setSource("field", "value"), + prepareIndex("test2").setId("4").setSource("field", "value"), + prepareIndex("test2").setId("5").setSource("field", "value"), + prepareIndex("test2").setId("6").setSource("field", "value") + ); + + assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 0); + } + + public void testSearchMultipleIndicesWithOneIndexRefreshBlock() { + createIndex("test"); + createIndex("test2"); + + // Only block test + var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); + client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); + + indexRandom( + true, + prepareIndex("test").setId("1").setSource("field", "value"), + prepareIndex("test").setId("2").setSource("field", "value"), + prepareIndex("test").setId("3").setSource("field", "value"), + prepareIndex("test").setId("4").setSource("field", "value"), + prepareIndex("test").setId("5").setSource("field", "value"), + prepareIndex("test").setId("6").setSource("field", "value"), + prepareIndex("test2").setId("1").setSource("field", "value"), + prepareIndex("test2").setId("2").setSource("field", "value"), + prepareIndex("test2").setId("3").setSource("field", "value"), + prepareIndex("test2").setId("4").setSource("field", "value"), + prepareIndex("test2").setId("5").setSource("field", "value"), + prepareIndex("test2").setId("6").setSource("field", "value") + ); + + // We should get test2 results (not blocked) + assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 6); + } + + public void testOpenPITWithIndexRefreshBlock() { + createIndex("test"); + + var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); + client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); + + indexRandom( + true, + prepareIndex("test").setId("1").setSource("field", "value"), + prepareIndex("test").setId("2").setSource("field", "value"), + prepareIndex("test").setId("3").setSource("field", "value"), + prepareIndex("test").setId("4").setSource("field", "value"), + prepareIndex("test").setId("5").setSource("field", "value"), + prepareIndex("test").setId("6").setSource("field", "value") + ); + + BytesReference pitId = null; + try { + OpenPointInTimeRequest openPITRequest = new OpenPointInTimeRequest("test").keepAlive(TimeValue.timeValueSeconds(10)) + .allowPartialSearchResults(true); + pitId = client().execute(TransportOpenPointInTimeAction.TYPE, openPITRequest).actionGet().getPointInTimeId(); + SearchRequest searchRequest = new SearchRequest().source( + new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder(pitId).setKeepAlive(TimeValue.timeValueSeconds(10))) + ); + SearchResponse searchResponse = client().search(searchRequest).actionGet(); + assertHitCount(searchResponse, 0); + } finally { + if (pitId != null) { + client().execute(TransportClosePointInTimeAction.TYPE, new ClosePointInTimeRequest(pitId)).actionGet(); + } + } + } +} diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java index 59ee04414b3fc..67cd3fe8b47fd 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java @@ -283,7 +283,8 @@ public enum APIBlock implements Writeable { READ("read", INDEX_READ_BLOCK, Property.ServerlessPublic), WRITE("write", INDEX_WRITE_BLOCK, Property.ServerlessPublic), METADATA("metadata", INDEX_METADATA_BLOCK, Property.ServerlessPublic), - READ_ONLY_ALLOW_DELETE("read_only_allow_delete", INDEX_READ_ONLY_ALLOW_DELETE_BLOCK); + READ_ONLY_ALLOW_DELETE("read_only_allow_delete", INDEX_READ_ONLY_ALLOW_DELETE_BLOCK), + REFRESH("refresh", INDEX_REFRESH_BLOCK); final String name; final String settingName; diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index 5cf5fc7f30eea..1fb28725ba4f0 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -1817,7 +1817,7 @@ public void onFailure(Exception ex) { } } - public void testSkippedIndicesWithRefreshBlock() { + public void testSkippedIteratorsForIndicesWithRefreshBlock() { final ProjectId projectId = randomProjectIdOrDefault(); String normalIndexName = "test-normal"; From be37bf63fb64a956abaf84fb0a47b3d9bf3c91a2 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 11 Jun 2025 16:26:58 -0400 Subject: [PATCH 09/18] Fix resource leak by using decRef assertion --- .../java/org/elasticsearch/search/SearchWithIndexBlocksIT.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index af7a25e1d5dc2..5f580f74eb732 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -126,8 +126,7 @@ public void testOpenPITWithIndexRefreshBlock() { SearchRequest searchRequest = new SearchRequest().source( new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder(pitId).setKeepAlive(TimeValue.timeValueSeconds(10))) ); - SearchResponse searchResponse = client().search(searchRequest).actionGet(); - assertHitCount(searchResponse, 0); + assertHitCount(client().search(searchRequest), 0); } finally { if (pitId != null) { client().execute(TransportClosePointInTimeAction.TYPE, new ClosePointInTimeRequest(pitId)).actionGet(); From 17706e23095df7ccbdb89a924fbd1e5f936b55a8 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Wed, 11 Jun 2025 20:37:22 +0000 Subject: [PATCH 10/18] [CI] Auto commit changes from spotless --- .../java/org/elasticsearch/search/SearchWithIndexBlocksIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index 5f580f74eb732..a5c99418189f0 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -14,7 +14,6 @@ import org.elasticsearch.action.search.ClosePointInTimeRequest; import org.elasticsearch.action.search.OpenPointInTimeRequest; import org.elasticsearch.action.search.SearchRequest; -import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.TransportClosePointInTimeAction; import org.elasticsearch.action.search.TransportOpenPointInTimeAction; import org.elasticsearch.cluster.metadata.IndexMetadata; From bf8a2be9705b7c706ab4275eab406d748998f3e3 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Mon, 30 Jun 2025 22:12:55 -0400 Subject: [PATCH 11/18] Improve names of valid shard check method and variables --- .../action/search/TransportSearchAction.java | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) 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 2a89faf8da31f..4060ec76abec2 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -148,6 +148,7 @@ public class TransportSearchAction extends HandledTransportAction concreteIndexBoosts = resolveIndexBoosts(searchRequest, projectState.cluster()); - boolean oneOrZeroValidShards = shardIterators.size() == 1 || allOrAllButOneSkipped(shardIterators); - adjustSearchType(searchRequest, oneOrZeroValidShards); + adjustSearchType(searchRequest, oneOrZeroShardsToSearch(shardIterators)); final DiscoveryNodes nodes = projectState.cluster().nodes(); BiFunction connectionLookup = buildConnectionLookup( @@ -1345,17 +1345,21 @@ private void executeSearch( } /** - * Determines if all, or all but one, iterators are skipped. + * Determines if only one (or zero) search shard iterators will be searched. * (At this point, iterators may be marked as skipped due to index level blockers). - * We expect skipped iteators to be unlikely, so returning fast after we see more + * We expect skipped iterators to be unlikely, so returning fast after we see more * than one "not skipped" is an intended optimization. * * @param searchShardIterators all the shard iterators derived from indices being searched - * @return true if all of them are already skipped, or only one is not skipped + * @return true if there are no more than one shard iterators, or if there are no more than + * one not marked to skip */ - private boolean allOrAllButOneSkipped(List searchShardIterators) { - int notSkippedCount = 0; + private boolean oneOrZeroShardsToSearch(List searchShardIterators) { + if (searchShardIterators.size() <= 1) { + return true; + } + int notSkippedCount = 0; for (SearchShardIterator searchShardIterator : searchShardIterators) { if (searchShardIterator.skip() == false) { notSkippedCount++; @@ -1364,7 +1368,6 @@ private boolean allOrAllButOneSkipped(List searchShardItera } } } - return true; } From 0f0200ab8e18035ba0752eafd6641bd04f688750 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Tue, 1 Jul 2025 12:35:34 -0400 Subject: [PATCH 12/18] Remove constructor used only in tests --- .../action/search/SearchShardIterator.java | 13 -------- .../AbstractSearchAsyncActionTests.java | 5 +-- .../action/search/SearchAsyncActionTests.java | 5 +-- .../search/SearchShardIteratorTests.java | 32 +++++++++++++++---- .../search/TransportSearchActionTests.java | 2 +- 5 files changed, 33 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 53d5e1f5b717f..45c58c5a64611 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -39,19 +39,6 @@ public final class SearchShardIterator implements Comparable targetNodesIterator; - /** - * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards - * for a given shardId. - * - * @param clusterAlias the alias of the cluster where the shard is located - * @param shardId shard id of the group - * @param shards shards to iterate - * @param originalIndices the indices that the search request originally related to (before any rewriting happened) - */ - public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List shards, OriginalIndices originalIndices) { - this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, false); - } - /** * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards * for a given shardId. diff --git a/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java index abe7e893977f4..b4a3cf3e61668 100644 --- a/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java @@ -82,7 +82,7 @@ private AbstractSearchAsyncAction createAction( null, request, listener, - Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null)), + Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null, false)), timeProvider, ClusterState.EMPTY_STATE, null, @@ -153,7 +153,8 @@ public void testBuildShardSearchTransportRequest() { clusterAlias, new ShardId(new Index("name", "foo"), 1), Collections.emptyList(), - new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand()) + new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand()), + false ); ShardSearchRequest shardSearchTransportRequest = action.buildShardSearchRequest(iterator, 10); assertEquals(IndicesOptions.strictExpand(), shardSearchTransportRequest.indicesOptions()); diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java index afd3bee4c4ab8..03d2a69c8b1f7 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java @@ -640,7 +640,8 @@ public void testSkipUnavailableSearchShards() throws InterruptedException { null, new ShardId(index, 0), Collections.emptyList(), - originalIndices + originalIndices, + false ); // Skip all the shards searchShardIterator.skip(true); @@ -760,7 +761,7 @@ static List getShardsIter( } Collections.shuffle(started, random()); started.addAll(initializing); - list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices)); + list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices, false)); } return list; } diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java index 79736427f634d..f16347b09d147 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java @@ -45,7 +45,13 @@ private static List randomShardRoutings(ShardId shardId, int numRe public void testShardId() { ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomInt()); - SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), OriginalIndices.NONE); + SearchShardIterator searchShardIterator = new SearchShardIterator( + null, + shardId, + Collections.emptyList(), + OriginalIndices.NONE, + false + ); assertSame(shardId, searchShardIterator.shardId()); } @@ -55,7 +61,7 @@ public void testGetOriginalIndices() { new String[] { randomAlphaOfLengthBetween(3, 10) }, IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean()) ); - SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices); + SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices, false); assertSame(originalIndices, searchShardIterator.getOriginalIndices()); } @@ -66,7 +72,8 @@ public void testGetClusterAlias() { clusterAlias, shardId, Collections.emptyList(), - OriginalIndices.NONE + OriginalIndices.NONE, + false ); assertEquals(clusterAlias, searchShardIterator.getClusterAlias()); } @@ -164,7 +171,13 @@ public void testCompareTo() { for (String uuid : uuids) { ShardId shardId = new ShardId(index, uuid, i); shardIterators.add( - new SearchShardIterator(null, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()) + new SearchShardIterator( + null, + shardId, + randomShardRoutings(shardId), + OriginalIndicesTests.randomOriginalIndices(), + false + ) ); for (String cluster : clusters) { shardIterators.add( @@ -172,7 +185,8 @@ public void testCompareTo() { cluster, shardId, randomShardRoutings(shardId), - OriginalIndicesTests.randomOriginalIndices() + OriginalIndicesTests.randomOriginalIndices(), + false ) ); } @@ -217,6 +231,12 @@ public void testCompareToEqualItems() { private static SearchShardIterator randomSearchShardIterator() { String clusterAlias = randomBoolean() ? null : randomAlphaOfLengthBetween(5, 10); ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomIntBetween(0, Integer.MAX_VALUE)); - return new SearchShardIterator(clusterAlias, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()); + return new SearchShardIterator( + clusterAlias, + shardId, + randomShardRoutings(shardId), + OriginalIndicesTests.randomOriginalIndices(), + false + ); } } diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index 85e341b0b82a0..fbe790dc8f55c 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -158,7 +158,7 @@ private static SearchShardIterator createSearchShardIterator( ) { ShardId shardId = new ShardId(index, id); List shardRoutings = SearchShardIteratorTests.randomShardRoutings(shardId); - return new SearchShardIterator(clusterAlias, shardId, shardRoutings, originalIndices); + return new SearchShardIterator(clusterAlias, shardId, shardRoutings, originalIndices, false); } private static ResolvedIndices createMockResolvedIndices( From 7689263e96aa20d8d71d16b6a037aee51cec6ccb Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Tue, 1 Jul 2025 12:47:10 -0400 Subject: [PATCH 13/18] Fix missed merge conflict --- .../elasticsearch/action/search/TransportSearchActionTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index fbe790dc8f55c..27381009159d3 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -1869,7 +1869,7 @@ public void testSkippedIteratorsForIndicesWithRefreshBlock() { transportVersion, threadPool ); - NodeClient client = new NodeClient(settings, threadPool); + NodeClient client = new NodeClient(settings, threadPool, TestProjectResolvers.alwaysThrow()); SearchService searchService = mock(SearchService.class); when(searchService.getRewriteContext(any(), any(), any(), anyBoolean())).thenReturn( new QueryRewriteContext(null, null, null, null, null, null) From 598e906437ad4afa3e6a11888e8c64425c0a457d Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Wed, 2 Jul 2025 21:50:48 -0400 Subject: [PATCH 14/18] Remove ability to set INDEX_REFRESH_BLOCK from API, add directly to cluster state in tests --- .../search/SearchWithIndexBlocksIT.java | 168 +++++++++--------- .../cluster/metadata/IndexMetadata.java | 3 +- 2 files changed, 82 insertions(+), 89 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index a5c99418189f0..2f9cde7c27bef 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -9,14 +9,18 @@ package org.elasticsearch.search; -import org.elasticsearch.action.admin.indices.readonly.AddIndexBlockRequest; -import org.elasticsearch.action.admin.indices.readonly.TransportAddIndexBlockAction; +import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.ClosePointInTimeRequest; import org.elasticsearch.action.search.OpenPointInTimeRequest; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.TransportClosePointInTimeAction; import org.elasticsearch.action.search.TransportOpenPointInTimeAction; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.cluster.metadata.ProjectId; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.query.QueryBuilders; @@ -24,112 +28,102 @@ import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.test.ESIntegTestCase; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.elasticsearch.cluster.block.ClusterBlocks.EMPTY_CLUSTER_BLOCK; +import static org.elasticsearch.test.ClusterServiceUtils.setState; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; public class SearchWithIndexBlocksIT extends ESIntegTestCase { - public void testSearchIndexWithIndexRefreshBlock() { - createIndex("test"); - - var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); - client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); - - indexRandom( - true, - prepareIndex("test").setId("1").setSource("field", "value"), - prepareIndex("test").setId("2").setSource("field", "value"), - prepareIndex("test").setId("3").setSource("field", "value"), - prepareIndex("test").setId("4").setSource("field", "value"), - prepareIndex("test").setId("5").setSource("field", "value"), - prepareIndex("test").setId("6").setSource("field", "value") - ); + public void testSearchIndicesWithIndexRefreshBlocks() { + List indices = createIndices(); + Map numDocsPerIndex = indexDocuments(indices); + List unblockedIndices = addIndexRefreshBlockToSomeIndices(indices); - assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 0); - } - - public void testSearchMultipleIndicesEachWithAnIndexRefreshBlock() { - createIndex("test"); - createIndex("test2"); - - var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test", "test2"); - client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); - - indexRandom( - true, - prepareIndex("test").setId("1").setSource("field", "value"), - prepareIndex("test").setId("2").setSource("field", "value"), - prepareIndex("test").setId("3").setSource("field", "value"), - prepareIndex("test").setId("4").setSource("field", "value"), - prepareIndex("test").setId("5").setSource("field", "value"), - prepareIndex("test").setId("6").setSource("field", "value"), - prepareIndex("test2").setId("1").setSource("field", "value"), - prepareIndex("test2").setId("2").setSource("field", "value"), - prepareIndex("test2").setId("3").setSource("field", "value"), - prepareIndex("test2").setId("4").setSource("field", "value"), - prepareIndex("test2").setId("5").setSource("field", "value"), - prepareIndex("test2").setId("6").setSource("field", "value") - ); - - assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 0); - } + int expectedHits = 0; + for (String index : unblockedIndices) { + expectedHits += numDocsPerIndex.get(index); + } - public void testSearchMultipleIndicesWithOneIndexRefreshBlock() { - createIndex("test"); - createIndex("test2"); - - // Only block test - var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); - client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); - - indexRandom( - true, - prepareIndex("test").setId("1").setSource("field", "value"), - prepareIndex("test").setId("2").setSource("field", "value"), - prepareIndex("test").setId("3").setSource("field", "value"), - prepareIndex("test").setId("4").setSource("field", "value"), - prepareIndex("test").setId("5").setSource("field", "value"), - prepareIndex("test").setId("6").setSource("field", "value"), - prepareIndex("test2").setId("1").setSource("field", "value"), - prepareIndex("test2").setId("2").setSource("field", "value"), - prepareIndex("test2").setId("3").setSource("field", "value"), - prepareIndex("test2").setId("4").setSource("field", "value"), - prepareIndex("test2").setId("5").setSource("field", "value"), - prepareIndex("test2").setId("6").setSource("field", "value") - ); - - // We should get test2 results (not blocked) - assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 6); + assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), expectedHits); } public void testOpenPITWithIndexRefreshBlock() { - createIndex("test"); - - var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test"); - client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet(); + List indices = createIndices(); + Map numDocsPerIndex = indexDocuments(indices); + List unblockedIndices = addIndexRefreshBlockToSomeIndices(indices); - indexRandom( - true, - prepareIndex("test").setId("1").setSource("field", "value"), - prepareIndex("test").setId("2").setSource("field", "value"), - prepareIndex("test").setId("3").setSource("field", "value"), - prepareIndex("test").setId("4").setSource("field", "value"), - prepareIndex("test").setId("5").setSource("field", "value"), - prepareIndex("test").setId("6").setSource("field", "value") - ); + int expectedHits = 0; + for (String index : unblockedIndices) { + expectedHits += numDocsPerIndex.get(index); + } BytesReference pitId = null; try { - OpenPointInTimeRequest openPITRequest = new OpenPointInTimeRequest("test").keepAlive(TimeValue.timeValueSeconds(10)) - .allowPartialSearchResults(true); + OpenPointInTimeRequest openPITRequest = new OpenPointInTimeRequest(indices.toArray(new String[0])).keepAlive( + TimeValue.timeValueSeconds(10) + ).allowPartialSearchResults(true); pitId = client().execute(TransportOpenPointInTimeAction.TYPE, openPITRequest).actionGet().getPointInTimeId(); SearchRequest searchRequest = new SearchRequest().source( new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder(pitId).setKeepAlive(TimeValue.timeValueSeconds(10))) ); - assertHitCount(client().search(searchRequest), 0); + assertHitCount(client().search(searchRequest), expectedHits); } finally { if (pitId != null) { client().execute(TransportClosePointInTimeAction.TYPE, new ClosePointInTimeRequest(pitId)).actionGet(); } } } + + private List createIndices() { + int numIndices = randomIntBetween(1, 3); + List indices = new ArrayList<>(); + for (int i = 0; i < numIndices; i++) { + indices.add("test" + i); + createIndex("test" + i); + } + return indices; + } + + private Map indexDocuments(List indices) { + Map numDocsPerIndex = new HashMap<>(); + List indexRequests = new ArrayList<>(); + for (String index : indices) { + int numDocs = randomIntBetween(0, 10); + numDocsPerIndex.put(index, numDocs); + for (int i = 0; i < numDocs; i++) { + indexRequests.add(prepareIndex(index).setId(String.valueOf(i)).setSource("field", "value")); + } + } + indexRandom(true, indexRequests); + + return numDocsPerIndex; + } + + private List addIndexRefreshBlockToSomeIndices(List indices) { + List unblockedIndices = new ArrayList<>(); + var blocksBuilder = ClusterBlocks.builder().blocks(EMPTY_CLUSTER_BLOCK); + for (String index : indices) { + boolean blockIndex = randomBoolean(); + if (blockIndex) { + blocksBuilder.addIndexBlock(ProjectId.DEFAULT, index, IndexMetadata.INDEX_REFRESH_BLOCK); + } else { + unblockedIndices.add(index); + } + } + + var dataNodes = clusterService().state().getNodes().getAllNodes(); + for (DiscoveryNode dataNode : dataNodes) { + ClusterService clusterService = internalCluster().getInstance(ClusterService.class, dataNode.getName()); + ClusterState currentState = clusterService.state(); + ClusterState newState = ClusterState.builder(currentState).blocks(blocksBuilder).build(); + setState(clusterService, newState); + } + + return unblockedIndices; + } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java index d611405f39257..ef29a74fd47a5 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java @@ -283,8 +283,7 @@ public enum APIBlock implements Writeable { READ("read", INDEX_READ_BLOCK, Property.ServerlessPublic), WRITE("write", INDEX_WRITE_BLOCK, Property.ServerlessPublic), METADATA("metadata", INDEX_METADATA_BLOCK, Property.ServerlessPublic), - READ_ONLY_ALLOW_DELETE("read_only_allow_delete", INDEX_READ_ONLY_ALLOW_DELETE_BLOCK), - REFRESH("refresh", INDEX_REFRESH_BLOCK); + READ_ONLY_ALLOW_DELETE("read_only_allow_delete", INDEX_READ_ONLY_ALLOW_DELETE_BLOCK); final String name; final String settingName; From 86c9a5df88c329104fd7dafa02fac18e2ef3973a Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Tue, 15 Jul 2025 11:03:28 -0400 Subject: [PATCH 15/18] Rework change to ignore blocked indices before shard resolution --- .../search/SearchWithIndexBlocksIT.java | 3 + .../search/CanMatchPreFilterSearchPhase.java | 6 - .../action/search/SearchShardIterator.java | 19 +-- .../action/search/TransportSearchAction.java | 64 +++------ .../AbstractSearchAsyncActionTests.java | 5 +- .../action/search/SearchAsyncActionTests.java | 5 +- .../search/SearchShardIteratorTests.java | 32 +---- .../search/TransportSearchActionTests.java | 123 ++++-------------- 8 files changed, 63 insertions(+), 194 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index 2f9cde7c27bef..5859a1712a9ef 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -9,6 +9,8 @@ package org.elasticsearch.search; +import com.carrotsearch.randomizedtesting.annotations.Repeat; + import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.ClosePointInTimeRequest; import org.elasticsearch.action.search.OpenPointInTimeRequest; @@ -37,6 +39,7 @@ import static org.elasticsearch.test.ClusterServiceUtils.setState; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; +@Repeat(iterations = 100) public class SearchWithIndexBlocksIT extends ESIntegTestCase { public void testSearchIndicesWithIndexRefreshBlocks() { 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 8adb9180e3bae..e42f8127c5e97 100644 --- a/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java @@ -186,12 +186,6 @@ private void runCoordinatorRewritePhase() { assert assertSearchCoordinationThread(); final List matchedShardLevelRequests = new ArrayList<>(); for (SearchShardIterator searchShardIterator : shardsIts) { - if (searchShardIterator.prefiltered() == false && searchShardIterator.skip()) { - // This implies the iterator was skipped due to an index level block, - // not a remote can-match run. - continue; - } - final CanMatchNodeRequest canMatchNodeRequest = new CanMatchNodeRequest( request, searchShardIterator.getOriginalIndices().indicesOptions(), diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 45c58c5a64611..128deba7844ca 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -41,28 +41,19 @@ public final class SearchShardIterator implements ComparableshardId. + * this the given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group * @param shards shards to iterate * @param originalIndices the indices that the search request originally related to (before any rewriting happened) - * @param skip if true, then this group won't have matches (due to an index level block), - * and it can be safely skipped from the search */ - public SearchShardIterator( - @Nullable String clusterAlias, - ShardId shardId, - List shards, - OriginalIndices originalIndices, - boolean skip - ) { - this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, skip); + public SearchShardIterator(@Nullable String clusterAlias, ShardId shardId, List shards, OriginalIndices originalIndices) { + this(clusterAlias, shardId, shards.stream().map(ShardRouting::currentNodeId).toList(), originalIndices, null, null, false, false); } /** * Creates a {@link SearchShardIterator} instance that iterates over a subset of the given shards - * for a given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group @@ -71,8 +62,7 @@ public SearchShardIterator( * @param searchContextId the point-in-time specified for this group if exists * @param searchContextKeepAlive the time interval that data nodes should extend the keep alive of the point-in-time * @param prefiltered if true, then this group already executed the can_match phase - * @param skip if true, then this group won't have matches (due to can match, or an index level block), - * and it can be safely skipped from the search + * @param skip if true, then this group won't have matches, and it can be safely skipped from the search */ public SearchShardIterator( @Nullable String clusterAlias, @@ -93,6 +83,7 @@ public SearchShardIterator( assert searchContextKeepAlive == null || searchContextId != null; this.prefiltered = prefiltered; this.skip = skip; + assert skip == false || prefiltered : "only prefiltered shards are skip-able"; } /** 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 d63f6dec9bfc7..23718e1460699 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -148,9 +148,6 @@ public class TransportSearchAction extends HandledTransportAction buildPerIndexOriginalIndices( for (String index : indices) { if (hasBlocks) { blocks.indexBlockedRaiseException(projectId, ClusterBlockLevel.READ, index); - if (blocks.hasIndexBlock(projectState.projectId(), index, IndexMetadata.INDEX_REFRESH_BLOCK)) { - res.put(index, SKIPPED_INDICES); - continue; - } } String[] aliases = indexNameExpressionResolver.allIndexAliases(projectState.metadata(), index, indicesAndAliases); @@ -596,7 +589,7 @@ public void onFailure(Exception e) {} ); } - static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShardsToSearch) { + static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShards) { // if there's a kNN search, always use DFS_QUERY_THEN_FETCH if (searchRequest.hasKnnSearch()) { searchRequest.searchType(DFS_QUERY_THEN_FETCH); @@ -611,7 +604,7 @@ static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShard } // optimize search type for cases where there is only one shard group to search on - if (oneOrZeroShardsToSearch) { + if (oneOrZeroShards) { // 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 searchRequest.searchType(QUERY_THEN_FETCH); } @@ -1312,7 +1305,7 @@ private void executeSearch( Map concreteIndexBoosts = resolveIndexBoosts(searchRequest, projectState.cluster()); - adjustSearchType(searchRequest, oneOrZeroShardsToSearch(shardIterators)); + adjustSearchType(searchRequest, shardIterators.size() <= 1); final DiscoveryNodes nodes = projectState.cluster().nodes(); BiFunction connectionLookup = buildConnectionLookup( @@ -1345,33 +1338,6 @@ private void executeSearch( ); } - /** - * Determines if only one (or zero) search shard iterators will be searched. - * (At this point, iterators may be marked as skipped due to index level blockers). - * We expect skipped iterators to be unlikely, so returning fast after we see more - * than one "not skipped" is an intended optimization. - * - * @param searchShardIterators all the shard iterators derived from indices being searched - * @return true if there are no more than one shard iterators, or if there are no more than - * one not marked to skip - */ - private boolean oneOrZeroShardsToSearch(List searchShardIterators) { - if (searchShardIterators.size() <= 1) { - return true; - } - - int notSkippedCount = 0; - for (SearchShardIterator searchShardIterator : searchShardIterators) { - if (searchShardIterator.skip() == false) { - notSkippedCount++; - if (notSkippedCount > 1) { - return false; - } - } - } - return true; - } - Executor asyncSearchExecutor(final String[] indices) { boolean seenSystem = false; boolean seenCritical = false; @@ -1898,6 +1864,7 @@ List getLocalShardsIterator( Set indicesAndAliases, String[] concreteIndices ) { + concreteIndices = ignoreBlockedIndices(projectState, concreteIndices); var routingMap = indexNameExpressionResolver.resolveSearchRouting( projectState.metadata(), searchRequest.routing(), @@ -1924,18 +1891,27 @@ List getLocalShardsIterator( final ShardId shardId = shardRouting.shardId(); OriginalIndices finalIndices = originalIndices.get(shardId.getIndex().getName()); assert finalIndices != null; - list[i++] = new SearchShardIterator( - clusterAlias, - shardId, - shardRouting.getShardRoutings(), - finalIndices, - finalIndices == SKIPPED_INDICES - ); + list[i++] = new SearchShardIterator(clusterAlias, shardId, shardRouting.getShardRoutings(), finalIndices); } // the returned list must support in-place sorting, so this is the most memory efficient we can do here return Arrays.asList(list); } + static String[] ignoreBlockedIndices(ProjectState projectState, String[] concreteIndices) { + // optimization: mostly we do not have any blocks so there's no point in the expensive per-index checking + boolean hasIndexBlocks = projectState.blocks().indices(projectState.projectId()).isEmpty() == false; + if (hasIndexBlocks) { + logger.info("Has index block: {}", projectState.blocks().toString()); + return Arrays.stream(concreteIndices) + .filter( + index -> projectState.blocks() + .hasIndexBlock(projectState.projectId(), index, IndexMetadata.INDEX_REFRESH_BLOCK) == false + ) + .toArray(String[]::new); + } + return concreteIndices; + } + private interface TelemetryListener { void setRemotes(int count); diff --git a/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java index b4a3cf3e61668..abe7e893977f4 100644 --- a/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java @@ -82,7 +82,7 @@ private AbstractSearchAsyncAction createAction( null, request, listener, - Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null, false)), + Collections.singletonList(new SearchShardIterator(null, new ShardId("index", "_na", 0), Collections.emptyList(), null)), timeProvider, ClusterState.EMPTY_STATE, null, @@ -153,8 +153,7 @@ public void testBuildShardSearchTransportRequest() { clusterAlias, new ShardId(new Index("name", "foo"), 1), Collections.emptyList(), - new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand()), - false + new OriginalIndices(new String[] { "name", "name1" }, IndicesOptions.strictExpand()) ); ShardSearchRequest shardSearchTransportRequest = action.buildShardSearchRequest(iterator, 10); assertEquals(IndicesOptions.strictExpand(), shardSearchTransportRequest.indicesOptions()); diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java index 03d2a69c8b1f7..afd3bee4c4ab8 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java @@ -640,8 +640,7 @@ public void testSkipUnavailableSearchShards() throws InterruptedException { null, new ShardId(index, 0), Collections.emptyList(), - originalIndices, - false + originalIndices ); // Skip all the shards searchShardIterator.skip(true); @@ -761,7 +760,7 @@ static List getShardsIter( } Collections.shuffle(started, random()); started.addAll(initializing); - list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices, false)); + list.add(new SearchShardIterator(null, new ShardId(index, i), started, originalIndices)); } return list; } diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java index f16347b09d147..79736427f634d 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchShardIteratorTests.java @@ -45,13 +45,7 @@ private static List randomShardRoutings(ShardId shardId, int numRe public void testShardId() { ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomInt()); - SearchShardIterator searchShardIterator = new SearchShardIterator( - null, - shardId, - Collections.emptyList(), - OriginalIndices.NONE, - false - ); + SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), OriginalIndices.NONE); assertSame(shardId, searchShardIterator.shardId()); } @@ -61,7 +55,7 @@ public void testGetOriginalIndices() { new String[] { randomAlphaOfLengthBetween(3, 10) }, IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean()) ); - SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices, false); + SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), originalIndices); assertSame(originalIndices, searchShardIterator.getOriginalIndices()); } @@ -72,8 +66,7 @@ public void testGetClusterAlias() { clusterAlias, shardId, Collections.emptyList(), - OriginalIndices.NONE, - false + OriginalIndices.NONE ); assertEquals(clusterAlias, searchShardIterator.getClusterAlias()); } @@ -171,13 +164,7 @@ public void testCompareTo() { for (String uuid : uuids) { ShardId shardId = new ShardId(index, uuid, i); shardIterators.add( - new SearchShardIterator( - null, - shardId, - randomShardRoutings(shardId), - OriginalIndicesTests.randomOriginalIndices(), - false - ) + new SearchShardIterator(null, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()) ); for (String cluster : clusters) { shardIterators.add( @@ -185,8 +172,7 @@ public void testCompareTo() { cluster, shardId, randomShardRoutings(shardId), - OriginalIndicesTests.randomOriginalIndices(), - false + OriginalIndicesTests.randomOriginalIndices() ) ); } @@ -231,12 +217,6 @@ public void testCompareToEqualItems() { private static SearchShardIterator randomSearchShardIterator() { String clusterAlias = randomBoolean() ? null : randomAlphaOfLengthBetween(5, 10); ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomIntBetween(0, Integer.MAX_VALUE)); - return new SearchShardIterator( - clusterAlias, - shardId, - randomShardRoutings(shardId), - OriginalIndicesTests.randomOriginalIndices(), - false - ); + return new SearchShardIterator(clusterAlias, shardId, randomShardRoutings(shardId), OriginalIndicesTests.randomOriginalIndices()); } } diff --git a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java index 27381009159d3..d25fbba71afbe 100644 --- a/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/TransportSearchActionTests.java @@ -41,8 +41,6 @@ import org.elasticsearch.cluster.node.VersionInformation; import org.elasticsearch.cluster.project.TestProjectResolvers; import org.elasticsearch.cluster.routing.IndexRoutingTable; -import org.elasticsearch.cluster.routing.OperationRouting; -import org.elasticsearch.cluster.routing.ShardIterator; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.TestShardRouting; @@ -135,8 +133,6 @@ import static org.hamcrest.Matchers.hasSize; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -158,7 +154,7 @@ private static SearchShardIterator createSearchShardIterator( ) { ShardId shardId = new ShardId(index, id); List shardRoutings = SearchShardIteratorTests.randomShardRoutings(shardId); - return new SearchShardIterator(clusterAlias, shardId, shardRoutings, originalIndices, false); + return new SearchShardIterator(clusterAlias, shardId, shardRoutings, originalIndices); } private static ResolvedIndices createMockResolvedIndices( @@ -1817,103 +1813,34 @@ public void onFailure(Exception ex) { } } - public void testSkippedIteratorsForIndicesWithRefreshBlock() { - final ProjectId projectId = randomProjectIdOrDefault(); - - String normalIndexName = "test-normal"; - String blockedIndexName = "test-blocked"; - final String[] indexNames = { normalIndexName, blockedIndexName }; - final Index normalIndex = new Index(normalIndexName, UUIDs.randomBase64UUID()); - final Index blockedIndex = new Index(blockedIndexName, UUIDs.randomBase64UUID()); - final int numberOfShards = randomIntBetween(1, 3); - final int numberOfReplicas = randomIntBetween(0, 1); - final int totalShards = numberOfShards + numberOfShards * numberOfReplicas; - - ClusterState clusterState = ClusterStateCreationUtils.stateWithAssignedPrimariesAndReplicas( - projectId, - indexNames, - numberOfShards, - numberOfReplicas - ); - ClusterBlocks.Builder blocksBuilder = ClusterBlocks.builder().blocks(clusterState.blocks()); - blocksBuilder.addIndexBlock(projectId, "test-blocked", IndexMetadata.INDEX_REFRESH_BLOCK); - clusterState = ClusterState.builder(clusterState).blocks(blocksBuilder).build(); - List shardIts = new ArrayList<>(); - for (int i = 0; i < totalShards; i++) { - shardIts.add(new ShardIterator(new ShardId(normalIndex, randomInt()), Collections.emptyList())); - shardIts.add(new ShardIterator(new ShardId(blockedIndex, randomInt()), Collections.emptyList())); + public void testIgnoreBlockedIndices() { + int numIndices = randomIntBetween(1, 10); + String[] concreteIndices = new String[numIndices]; + for (int i = 0; i < numIndices; i++) { + concreteIndices[i] = "index" + i; } - final OperationRouting operationRouting = mock(OperationRouting.class); - when( - operationRouting.searchShards( - eq(clusterState.projectState(projectId)), - eq(indexNames), - any(), - nullable(String.class), - any(), - any() - ) - ).thenReturn(shardIts); - ClusterService clusterService = mock(ClusterService.class); - when(clusterService.state()).thenReturn(clusterState); - when(clusterService.getSettings()).thenReturn(Settings.EMPTY); - when(clusterService.operationRouting()).thenReturn(operationRouting); - Settings settings = Settings.builder().put("node.name", TransportSearchAction.class.getSimpleName()).build(); - TransportVersion transportVersion = TransportVersionUtils.getNextVersion(TransportVersions.MINIMUM_CCS_VERSION, true); - ThreadPool threadPool = new ThreadPool(settings, MeterRegistry.NOOP, new DefaultBuiltInExecutorBuilders()); - try { - TransportService transportService = MockTransportService.createNewService( - Settings.EMPTY, - VersionInformation.CURRENT, - transportVersion, - threadPool - ); - NodeClient client = new NodeClient(settings, threadPool, TestProjectResolvers.alwaysThrow()); - SearchService searchService = mock(SearchService.class); - when(searchService.getRewriteContext(any(), any(), any(), anyBoolean())).thenReturn( - new QueryRewriteContext(null, null, null, null, null, null) - ); + List shuffledIndices = Arrays.asList(concreteIndices); + Collections.shuffle(shuffledIndices, random()); + concreteIndices = shuffledIndices.toArray(new String[0]); - TransportSearchAction transportSearchAction = new TransportSearchAction( - threadPool, - new NoneCircuitBreakerService(), - transportService, - searchService, - null, - new SearchTransportService(transportService, client, null), - null, - clusterService, - new ActionFilters(Collections.emptySet()), - TestProjectResolvers.usingRequestHeader(threadPool.getThreadContext()), - TestIndexNameExpressionResolver.newInstance(threadPool.getThreadContext()), - null, - null, - new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry()), - client, - new UsageService() - ); + final ProjectId projectId = randomProjectIdOrDefault(); + ClusterBlocks.Builder blocksBuilder = ClusterBlocks.builder(); + int numBlockedIndices = randomIntBetween(0, numIndices); + for (int i = 0; i < numBlockedIndices; i++) { + blocksBuilder.addIndexBlock(projectId, concreteIndices[i], IndexMetadata.INDEX_REFRESH_BLOCK); + } + final ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) + .putProjectMetadata(ProjectMetadata.builder(projectId).build()) + .blocks(blocksBuilder) + .build(); + final ProjectState projectState = clusterState.projectState(projectId); - SearchRequest searchRequest = new SearchRequest(indexNames); - searchRequest.allowPartialSearchResults(true); - List searchShardIts = transportSearchAction.getLocalShardsIterator( - clusterState.projectState(projectId), - searchRequest, - searchRequest.getLocalClusterAlias(), - new HashSet<>(), - indexNames - ); + String[] actual = TransportSearchAction.ignoreBlockedIndices(projectState, concreteIndices); + String[] expected = Arrays.stream(concreteIndices) + .filter(index -> clusterState.blocks().hasIndexBlock(projectId, index, IndexMetadata.INDEX_REFRESH_BLOCK) == false) + .toArray(String[]::new); - assertThat(searchShardIts.size(), equalTo(shardIts.size())); - for (SearchShardIterator searchShardIt : searchShardIts) { - if (searchShardIt.skip()) { - assertThat(searchShardIt.shardId().getIndexName(), equalTo("test-blocked")); - } else { - assertThat(searchShardIt.shardId().getIndexName(), equalTo("test-normal")); - } - } - } finally { - assertTrue(ESTestCase.terminate(threadPool)); - } + assertThat(Arrays.asList(actual), containsInAnyOrder(expected)); } } From d72600b9785110a06dd7b615f56cb8f56e69660d Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Tue, 15 Jul 2025 11:06:00 -0400 Subject: [PATCH 16/18] Clean up --- .../java/org/elasticsearch/search/SearchWithIndexBlocksIT.java | 3 --- .../org/elasticsearch/action/search/SearchShardIterator.java | 2 +- .../org/elasticsearch/action/search/TransportSearchAction.java | 1 - 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index 5859a1712a9ef..2f9cde7c27bef 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -9,8 +9,6 @@ package org.elasticsearch.search; -import com.carrotsearch.randomizedtesting.annotations.Repeat; - import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.ClosePointInTimeRequest; import org.elasticsearch.action.search.OpenPointInTimeRequest; @@ -39,7 +37,6 @@ import static org.elasticsearch.test.ClusterServiceUtils.setState; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; -@Repeat(iterations = 100) public class SearchWithIndexBlocksIT extends ESIntegTestCase { public void testSearchIndicesWithIndexRefreshBlocks() { diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java index 128deba7844ca..00ff8f33f5659 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java @@ -41,7 +41,7 @@ public final class SearchShardIterator implements ComparableshardId. + * this the a given shardId. * * @param clusterAlias the alias of the cluster where the shard is located * @param shardId shard id of the group 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 23718e1460699..41ae9dbb538df 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -1901,7 +1901,6 @@ static String[] ignoreBlockedIndices(ProjectState projectState, String[] concret // optimization: mostly we do not have any blocks so there's no point in the expensive per-index checking boolean hasIndexBlocks = projectState.blocks().indices(projectState.projectId()).isEmpty() == false; if (hasIndexBlocks) { - logger.info("Has index block: {}", projectState.blocks().toString()); return Arrays.stream(concreteIndices) .filter( index -> projectState.blocks() From 61d40c47d79b03629eb8b88a22db9ec4a7d81960 Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Tue, 15 Jul 2025 12:59:11 -0400 Subject: [PATCH 17/18] Add _msearch test case --- .../search/SearchWithIndexBlocksIT.java | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java index 2f9cde7c27bef..822779e9e6dbf 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchWithIndexBlocksIT.java @@ -32,10 +32,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import static org.elasticsearch.cluster.block.ClusterBlocks.EMPTY_CLUSTER_BLOCK; import static org.elasticsearch.test.ClusterServiceUtils.setState; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse; public class SearchWithIndexBlocksIT extends ESIntegTestCase { @@ -52,7 +54,7 @@ public void testSearchIndicesWithIndexRefreshBlocks() { assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), expectedHits); } - public void testOpenPITWithIndexRefreshBlock() { + public void testOpenPITOnIndicesWithIndexRefreshBlocks() { List indices = createIndices(); Map numDocsPerIndex = indexDocuments(indices); List unblockedIndices = addIndexRefreshBlockToSomeIndices(indices); @@ -79,6 +81,28 @@ public void testOpenPITWithIndexRefreshBlock() { } } + public void testMultiSearchIndicesWithIndexRefreshBlocks() { + List indices = createIndices(); + Map numDocsPerIndex = indexDocuments(indices); + List unblockedIndices = addIndexRefreshBlockToSomeIndices(indices); + + int expectedHits = 0; + for (String index : unblockedIndices) { + expectedHits += numDocsPerIndex.get(index); + } + + final long expectedHitsL = expectedHits; + assertResponse( + client().prepareMultiSearch() + .add(prepareSearch().setQuery(QueryBuilders.matchAllQuery())) + .add(prepareSearch().setQuery(QueryBuilders.termQuery("field", "blah"))), + response -> { + assertHitCount(Objects.requireNonNull(response.getResponses()[0].getResponse()), expectedHitsL); + assertHitCount(Objects.requireNonNull(response.getResponses()[1].getResponse()), 0); + } + ); + } + private List createIndices() { int numIndices = randomIntBetween(1, 3); List indices = new ArrayList<>(); From 51d5196a668d24373312eb5f28c27770558e05ff Mon Sep 17 00:00:00 2001 From: Ben Chaplin Date: Fri, 18 Jul 2025 10:07:52 -0400 Subject: [PATCH 18/18] Revert search type rewrite change --- .../elasticsearch/action/search/TransportSearchAction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 41ae9dbb538df..d4a41fbd038da 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -589,7 +589,7 @@ public void onFailure(Exception e) {} ); } - static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShards) { + static void adjustSearchType(SearchRequest searchRequest, boolean singleShard) { // if there's a kNN search, always use DFS_QUERY_THEN_FETCH if (searchRequest.hasKnnSearch()) { searchRequest.searchType(DFS_QUERY_THEN_FETCH); @@ -604,7 +604,7 @@ static void adjustSearchType(SearchRequest searchRequest, boolean oneOrZeroShard } // optimize search type for cases where there is only one shard group to search on - if (oneOrZeroShards) { + if (singleShard) { // 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 searchRequest.searchType(QUERY_THEN_FETCH); } @@ -1305,7 +1305,7 @@ private void executeSearch( Map concreteIndexBoosts = resolveIndexBoosts(searchRequest, projectState.cluster()); - adjustSearchType(searchRequest, shardIterators.size() <= 1); + adjustSearchType(searchRequest, shardIterators.size() == 1); final DiscoveryNodes nodes = projectState.cluster().nodes(); BiFunction connectionLookup = buildConnectionLookup(