From f01b866e276bc6979dcbecf25dfed04f05cc50d7 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 18 Feb 2025 09:41:48 +0100 Subject: [PATCH 1/5] Simplify logic around missing shards check in search phases This removes a couple of indirections: the error message for missing shards is always the same no matter the search phase. --- .../search/CanMatchPreFilterSearchPhase.java | 2 +- .../action/search/SearchPhase.java | 20 +++---------------- 2 files changed, 4 insertions(+), 18 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 f2f5b6df35711..92cadfd1e1a6d 100644 --- a/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java @@ -201,7 +201,7 @@ private synchronized void consumeResult(int shardIndex, boolean canMatch, MinAnd private void checkNoMissingShards(List shards) { assert assertSearchCoordinationThread(); - SearchPhase.doCheckNoMissingShards("can_match", request, shards, SearchPhase::makeMissingShardsError); + SearchPhase.doCheckNoMissingShards("can_match", request, shards); } private Map> groupByNode(List shards) { diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java b/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java index 1308a2fb61cfb..b46937ea975e9 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java @@ -14,7 +14,6 @@ import java.util.List; import java.util.Objects; -import java.util.function.Function; /** * Base class for all individual search phases like collecting distributed frequencies, fetching documents, querying shards. @@ -35,26 +34,13 @@ public String getName() { return name; } - protected String missingShardsErrorMessage(StringBuilder missingShards) { - return makeMissingShardsError(missingShards); - } - - protected static String makeMissingShardsError(StringBuilder missingShards) { + private static String makeMissingShardsError(StringBuilder missingShards) { return "Search rejected due to missing shards [" + missingShards + "]. Consider using `allow_partial_search_results` setting to bypass this error."; } - protected void doCheckNoMissingShards(String phaseName, SearchRequest request, List shardsIts) { - doCheckNoMissingShards(phaseName, request, shardsIts, this::missingShardsErrorMessage); - } - - protected static void doCheckNoMissingShards( - String phaseName, - SearchRequest request, - List shardsIts, - Function makeErrorMessage - ) { + protected static void doCheckNoMissingShards(String phaseName, SearchRequest request, List shardsIts) { assert request.allowPartialSearchResults() != null : "SearchRequest missing setting for allowPartialSearchResults"; if (request.allowPartialSearchResults() == false) { final StringBuilder missingShards = new StringBuilder(); @@ -70,7 +56,7 @@ protected static void doCheckNoMissingShards( } if (missingShards.isEmpty() == false) { // Status red - shard is missing all copies and would produce partial results for an index search - final String msg = makeErrorMessage.apply(missingShards); + final String msg = makeMissingShardsError(missingShards); throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY); } } From 9385176151abfa57eb1e7fd6fe998ff62bf29563 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 18 Feb 2025 10:00:12 +0100 Subject: [PATCH 2/5] iter --- .../elasticsearch/search/retriever/RetrieverRewriteIT.java | 2 +- .../action/search/TransportOpenPointInTimeAction.java | 6 ------ .../xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java | 2 +- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java index 25b43a2dc946e..650af9f6a82f8 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java @@ -129,7 +129,7 @@ public void testRewriteCompoundRetrieverShouldThrowForPartialResults() throws Ex ); assertThat( ex.getDetailedMessage(), - containsString("[open_point_in_time] action requires all shards to be available. Missing shards") + containsString("Search rejected due to missing shards") ); } finally { internalCluster().restartNode(randomDataNode); diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java index b8d0a928e05aa..43f989c5efdb8 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -241,12 +241,6 @@ void runOpenPointInTimePhase( searchRequest.getMaxConcurrentShardRequests(), clusters ) { - protected String missingShardsErrorMessage(StringBuilder missingShards) { - return "[open_point_in_time] action requires all shards to be available. Missing shards: [" - + missingShards - + "]. Consider using `allow_partial_search_results` setting to bypass this error."; - } - @Override protected void executePhaseOnShard( SearchShardIterator shardIt, diff --git a/x-pack/plugin/sql/qa/jdbc/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java b/x-pack/plugin/sql/qa/jdbc/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java index 0e0f7dc9722d9..f83047411f0b0 100644 --- a/x-pack/plugin/sql/qa/jdbc/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java +++ b/x-pack/plugin/sql/qa/jdbc/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java @@ -76,7 +76,7 @@ public void createTestIndex() throws IOException { public void testPartialResponseHandling() throws SQLException { try (Connection c = esJdbc(); Statement s = c.createStatement()) { SQLException exception = expectThrows(SQLException.class, () -> s.executeQuery("SELECT * FROM test ORDER BY test_field ASC")); - assertThat(exception.getMessage(), containsString("[open_point_in_time] action requires all shards to be available")); + assertThat(exception.getMessage(), containsString("Search rejected due to missing shards")); } } } From 02623e0880269354b3dcbe4d88545780e5fc7ea0 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 18 Feb 2025 09:16:07 +0000 Subject: [PATCH 3/5] [CI] Auto commit changes from spotless --- .../elasticsearch/search/retriever/RetrieverRewriteIT.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java index 650af9f6a82f8..bbe27fab2938a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java @@ -127,10 +127,7 @@ public void testRewriteCompoundRetrieverShouldThrowForPartialResults() throws Ex SearchPhaseExecutionException.class, client().prepareSearch(testIndex).setSource(source)::get ); - assertThat( - ex.getDetailedMessage(), - containsString("Search rejected due to missing shards") - ); + assertThat(ex.getDetailedMessage(), containsString("Search rejected due to missing shards")); } finally { internalCluster().restartNode(randomDataNode); } From e56d338c826d2615f4007d909ed3eabae77a2314 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 18 Feb 2025 18:24:31 +0100 Subject: [PATCH 4/5] Simplify logic around missing shards check in search phases This removes a couple of indirections: the error message for missing shards is always the same no matter the search phase. This was required to provide a slightly different error message for open PIT. The previous error was misleading when open PIT did not support setting allow_partial_search_results, but now that it does, it looks like we can unify the error message and simplify the code around it. --- .../elasticsearch/search/retriever/RetrieverRewriteIT.java | 5 ++++- .../xpack/sql/qa/single_node/JdbcShardFailureIT.java | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java index bbe27fab2938a..650af9f6a82f8 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java @@ -127,7 +127,10 @@ public void testRewriteCompoundRetrieverShouldThrowForPartialResults() throws Ex SearchPhaseExecutionException.class, client().prepareSearch(testIndex).setSource(source)::get ); - assertThat(ex.getDetailedMessage(), containsString("Search rejected due to missing shards")); + assertThat( + ex.getDetailedMessage(), + containsString("Search rejected due to missing shards") + ); } finally { internalCluster().restartNode(randomDataNode); } diff --git a/x-pack/plugin/sql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcShardFailureIT.java b/x-pack/plugin/sql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcShardFailureIT.java index f7d08ba4e22dd..a554e62d0c6d4 100644 --- a/x-pack/plugin/sql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcShardFailureIT.java +++ b/x-pack/plugin/sql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcShardFailureIT.java @@ -89,7 +89,7 @@ public void testPartialResponseHandling() throws Exception { createTestIndex(); try (Connection c = esJdbc(); Statement s = c.createStatement()) { SQLException exception = expectThrows(SQLException.class, () -> s.executeQuery("SELECT * FROM test ORDER BY test_field ASC")); - assertThat(exception.getMessage(), containsString("[open_point_in_time] action requires all shards to be available")); + assertThat(exception.getMessage(), containsString("Search rejected due to missing shards")); } } From 85b0d1dae1177c89bb95b84c720f392a7c14c4bf Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 18 Feb 2025 17:31:09 +0000 Subject: [PATCH 5/5] [CI] Auto commit changes from spotless --- .../elasticsearch/search/retriever/RetrieverRewriteIT.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java index 650af9f6a82f8..bbe27fab2938a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/retriever/RetrieverRewriteIT.java @@ -127,10 +127,7 @@ public void testRewriteCompoundRetrieverShouldThrowForPartialResults() throws Ex SearchPhaseExecutionException.class, client().prepareSearch(testIndex).setSource(source)::get ); - assertThat( - ex.getDetailedMessage(), - containsString("Search rejected due to missing shards") - ); + assertThat(ex.getDetailedMessage(), containsString("Search rejected due to missing shards")); } finally { internalCluster().restartNode(randomDataNode); }