Skip search shards with INDEX_REFRESH_BLOCK #129132

benchaplin · 2025-06-09T05:56:43Z

#117543 introduced a ClusterBlock which is applied to new indices in Serverless which do not yet have search shards up. We should skip searches for indices with this block in order to avoid meaningless 503s.

Edit: I've changed approaches to this "skipping" logic and wanted to document some reasoning:

I considered two options:

Hold the same search shard iterators, but mark them as "skipped"
Skip the index before resolving search shard iterators

Option 1 would result in a search response with {"_shards": { "skipped": = the number of shards we skipped due to an index refresh block. Option 2 pretends those shards don't exist, so {"_shards": { "skipped": 0, "total": < total in option 1.

I've decided to go with option 2. The reason is that this cluster block is only active when the index was just created and is still in a red state. I could not think of a reason that a user would need to be aware of the number of skipped shards in this case (a user curious about this detail would likely be able to deduce what's going on anyway). Furthermore, it simplifies the code change (see 86c9a5d).

elasticsearchmachine · 2025-06-09T05:57:12Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

tlrx

Left some comments. The search part should be reviewed by the ES Search team.

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

tlrx · 2025-06-12T09:13:29Z

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

+        var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test");
+        client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet();


The refresh block should be added automatically to newly created indices as long as they have replicas and the "use refresh block" setting is enabled in the node setting. We should remove the ability to add the refresh block through the Add Index Block API.

Thanks for taking a look @tlrx!

I was hoping to test this change outside of the context of Serverless. But I agree it's not appropriate to add the refresh block to that API for testing purposes only, so I will see if I can construct the scenario in some other way.

Alright, I was able to get the setup I was looking for by adding the block directly to cluster state in the tests.

tlrx · 2025-06-12T09:16:17Z

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

+        assertHitCount(prepareSearch().setQuery(QueryBuilders.matchAllQuery()), 0);
+    }
+
+    public void testSearchMultipleIndicesEachWithAnIndexRefreshBlock() {


I think this could be folded into a single test, where one or more indices are randomly created, most of some with replicas but other without replicas, and then allocate zero or more search shards and check the expected results, finally assigning all search shards and check the results again.

I've folded this into a single test with some additional randomization. My goal is to keep the integration tests in the Serverless PR, so I'll add the test scenario you're proposing there.

cbuescher

I did a first pass on the search related side of things and left a few questions and comments.

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

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

cbuescher

@benchaplin thanks for the last changes, I left one more small comment but nothing that should block this PR from my end. I didn't take a closer look at the tests since @tlrx seems to have made a few suggestions there, LGTM from my end though.

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

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

…luster state in tests

tlrx

LGTM but I know nothing about search shard iterators :)

tlrx · 2025-07-07T09:18:47Z

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

+            ClusterService clusterService = internalCluster().getInstance(ClusterService.class, dataNode.getName());
+            ClusterState currentState = clusterService.state();
+            ClusterState newState = ClusterState.builder(currentState).blocks(blocksBuilder).build();
+            setState(clusterService, newState);


This method is not intended to be used in integration test as it overrides the current data node cluster state.

For testing the INDEX_REFRESH_BLOCK I think it makes sense to only have unit tests in stateful elasticsearch.

Sorry @tlrx, can you explain more the risks of doing this?

I like having fine-grained control over blocks so I can write tests that block some indices and allow others in one search - I think this is critical to test. If I can only 'set' the block by controlling active search nodes (like I do in the other PR), I can't think of a way to achieve what I want.

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

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

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

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

javanna · 2025-07-10T15:26:11Z

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

@@ -234,6 +237,10 @@ private Map<String, OriginalIndices> 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);


I wonder if we are doing the filtering in the right place. It is a bit counter intuitive that we would resolve the shards given the indices and the skip some of them. Can we not filter the indices to start with? Maybe that removes the need to use that SKIPPED_INDICES marker thing too :)

Do we need to check for this block only in the search API?

By the way, something probably needs to happen in ES|QL too around this (does not need to be in this PR, but I am raising the need to track that).

I've adjusted my approach based on our discussion (documented in the PR description). Now I ignore blocked indices before resolving shards.

Do we need to check for this block only in the search API?

I've added a test to prove that _msearch hits this code path. Are you asking about other 'search-flavored' APIs?

I believe this will work for ES|QL, as EsqlSearchShardsAction calls TransportSearchShardAction which calls TransportSearchAction#getLocalShardsIterator. But thanks for bringing that up, I'll make sure there's a follow up task created to test this in ES|QL and make any additional changes necessary.

smalyshev · 2025-07-15T18:12:51Z

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

+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
+
+public class SearchWithIndexBlocksIT extends ESIntegTestCase {


Do we also want to have ESQL test for this case?

I'm tracking that to be a followup task.

javanna

I left a small question, I like how small the change has become!

javanna · 2025-07-18T13:31:46Z

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

@@ -1305,7 +1305,7 @@ private void executeSearch(

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

-        adjustSearchType(searchRequest, shardIterators.size() == 1);
+        adjustSearchType(searchRequest, shardIterators.size() <= 1);


is this change required? Is it necessary to adjust the search type where there's no shards to query?

Ah good question. I just tested it - looks like no. I had made the change to avoid a DfsQueryPhase error I was getting during the previous approach.

I think it's a meaningless change now, if there are no shards the search will end in AbstractSearchAsyncAction#start. I've removed it.

benchaplin added 5 commits June 4, 2025 19:00

Skip indices that have an index refresh block

2aa74e3

Merge branch 'main' into skip_search_shards_with_index_block

12b6b81

Construct the iterator skipped

9c705cd

Fix javadocs

1c75721

Add unit test

1ecc447

benchaplin requested a review from tlrx June 9, 2025 05:56

benchaplin added >non-issue Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations v9.1.0 labels Jun 9, 2025

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jun 9, 2025

benchaplin requested a review from a team as a code owner June 9, 2025 14:39

github-actions bot deployed to docs-preview June 9, 2025 14:40 View deployment

benchaplin force-pushed the skip_search_shards_with_index_block branch from 998cdd5 to 1ecc447 Compare June 9, 2025 17:05

benchaplin removed the request for review from a team June 9, 2025 17:06

elasticsearchmachine and others added 9 commits June 9, 2025 17:13

[CI] Auto commit changes from spotless

cdb4bc1

Merge branch 'main' into skip_search_shards_with_index_block

b7ade2d

Merge branch 'main' into skip_search_shards_with_index_block

cd991c2

Merge branch 'main' into skip_search_shards_with_index_block

5f50d5c

Rewrite DFS if processing one or zero unskipped shard iterators

3f86fb8

Make can-match support already skipped shard iterators

0edc27c

Add IT for executing search and PIT against refresh blocked indices

9de6f06

Fix resource leak by using decRef assertion

be37bf6

[CI] Auto commit changes from spotless

17706e2

benchaplin requested review from a team as code owners June 11, 2025 21:03

benchaplin force-pushed the skip_search_shards_with_index_block branch from e233cc7 to 17706e2 Compare June 11, 2025 21:04

Merge branch 'main' into skip_search_shards_with_index_block

8759a07

tlrx reviewed Jun 12, 2025

View reviewed changes

cbuescher reviewed Jun 18, 2025

View reviewed changes

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

benchaplin added 2 commits June 30, 2025 22:12

Improve names of valid shard check method and variables

bf8a2be

Merge branch 'main' into skip_search_shards_with_index_block

8887609

cbuescher approved these changes Jul 1, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/action/search/SearchShardIterator.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java Outdated Show resolved Hide resolved

benchaplin added 5 commits July 1, 2025 12:35

Remove constructor used only in tests

0f0200a

Fix missed merge conflict

7689263

Remove ability to set INDEX_REFRESH_BLOCK from API, add directly to c…

598e906

…luster state in tests

Merge branch 'main' into skip_search_shards_with_index_block

4cdfbd0

Merge branch 'main' into skip_search_shards_with_index_block

76ecade

tlrx reviewed Jul 7, 2025

View reviewed changes

javanna reviewed Jul 10, 2025

View reviewed changes

benchaplin added 4 commits July 15, 2025 11:03

Rework change to ignore blocked indices before shard resolution

86c9a5d

Merge branch 'main' into skip_search_shards_with_index_block

7200edc

Clean up

d72600b

Add _msearch test case

61d40c4

smalyshev reviewed Jul 15, 2025

View reviewed changes

rudolf mentioned this pull request Jul 16, 2025

[docs] ES tutorial on index initialization elastic/kibana#224628

Merged

9 tasks

benchaplin requested a review from javanna July 17, 2025 15:15

javanna reviewed Jul 18, 2025

View reviewed changes

Revert search type rewrite change

51d5196

benchaplin removed request for a team July 18, 2025 14:09

Merge branch 'main' into skip_search_shards_with_index_block

24f2770

		var addIndexBlockRequest = new AddIndexBlockRequest(IndexMetadata.APIBlock.REFRESH, "test");
		client().execute(TransportAddIndexBlockAction.TYPE, addIndexBlockRequest).actionGet();

Skip search shards with INDEX_REFRESH_BLOCK #129132

Are you sure you want to change the base?

Skip search shards with INDEX_REFRESH_BLOCK #129132

Conversation

benchaplin commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 9, 2025

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benchaplin commented Jun 9, 2025 •

edited

Loading