From 608607d7cb6b83e6356212509518fc221646dc0e Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Fri, 26 Jul 2024 07:59:30 +0300 Subject: [PATCH 1/6] updating error message for missing shards in open_point_in_time_action --- .../action/search/SearchPhase.java | 2 +- .../TransportOpenPointInTimeAction.java | 26 +++++++++++++++++++ .../jdbc/single_node/JdbcShardFailureIT.java | 5 +++- .../qa/single_node/JdbcShardFailureIT.java | 5 +++- 4 files changed, 35 insertions(+), 3 deletions(-) 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 7ad81154691c0..4e46858123c03 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java @@ -42,7 +42,7 @@ public void start() { } } - static void doCheckNoMissingShards(String phaseName, SearchRequest request, GroupShardsIterator shardsIts) { + protected void doCheckNoMissingShards(String phaseName, SearchRequest request, GroupShardsIterator shardsIts) { assert request.allowPartialSearchResults() != null : "SearchRequest missing setting for allowPartialSearchResults"; if (request.allowPartialSearchResults() == false) { final StringBuilder missingShards = new StringBuilder(); 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 91784ba331857..18c382a45f9f9 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -213,6 +213,32 @@ SearchPhase openPointInTimePhase( searchRequest.getMaxConcurrentShardRequests(), clusters ) { + @Override + protected void doCheckNoMissingShards( + String phaseName, + SearchRequest request, + GroupShardsIterator shardsIts + ) { + final StringBuilder missingShards = new StringBuilder(); + // Fail-fast verification of all shards being available + for (int index = 0; index < shardsIts.size(); index++) { + final SearchShardIterator shardRoutings = shardsIts.get(index); + if (shardRoutings.size() == 0) { + if (missingShards.isEmpty() == false) { + missingShards.append(", "); + } + missingShards.append(shardRoutings.shardId()); + } + } + if (missingShards.isEmpty() == false) { + // Status red - shard is missing all copies and would produce partial results for an index search + final String msg = "Search rejected - cannot execute [open_point_in_time_action] due to missing shards [" + + missingShards + + "]."; + throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY); + } + } + @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 f83047411f0b0..a9a4869e8d78f 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,10 @@ 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("Search rejected due to missing shards")); + assertThat( + exception.getMessage(), + containsString("Search rejected - cannot execute [open_point_in_time_action] due to missing shards") + ); } } } 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 dc9989b26c3b2..95aeb44d86436 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,10 @@ 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("Search rejected due to missing shards")); + assertThat( + exception.getMessage(), + containsString("Search rejected - cannot execute [open_point_in_time_action] due to missing shards") + ); } } From 8955591ccac64232919592a76121942fbbbf8868 Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Fri, 26 Jul 2024 08:01:46 +0300 Subject: [PATCH 2/6] iter --- .../action/search/TransportOpenPointInTimeAction.java | 4 +--- .../xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java | 5 +---- .../xpack/sql/qa/single_node/JdbcShardFailureIT.java | 5 +---- 3 files changed, 3 insertions(+), 11 deletions(-) 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 18c382a45f9f9..59c348d751c5d 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -232,9 +232,7 @@ protected 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 = "Search rejected - cannot execute [open_point_in_time_action] due to missing shards [" - + missingShards - + "]."; + final String msg = "Cannot execute [open_point_in_time] action due to missing shards [" + missingShards + "]."; throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY); } } 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 a9a4869e8d78f..646750fe56d35 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,10 +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("Search rejected - cannot execute [open_point_in_time_action] due to missing shards") - ); + assertThat(exception.getMessage(), containsString("Cannot execute [open_point_in_time] action due to missing shards")); } } } 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 95aeb44d86436..2a4b547f095ab 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,10 +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("Search rejected - cannot execute [open_point_in_time_action] due to missing shards") - ); + assertThat(exception.getMessage(), containsString("Cannot execute [open_point_in_time] action due to missing shards")); } } From f8c49ae5437245d5757390eedff1c9086ba66636 Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Fri, 26 Jul 2024 08:14:52 +0300 Subject: [PATCH 3/6] iter --- .../action/search/SearchPhase.java | 11 ++++++--- .../TransportOpenPointInTimeAction.java | 23 ++----------------- 2 files changed, 10 insertions(+), 24 deletions(-) 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 4e46858123c03..cb940d29efdab 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.io.UncheckedIOException; +import java.util.List; import java.util.Objects; /** @@ -42,6 +43,12 @@ public void start() { } } + protected String missingShardsErrorMessage(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, GroupShardsIterator shardsIts) { assert request.allowPartialSearchResults() != null : "SearchRequest missing setting for allowPartialSearchResults"; if (request.allowPartialSearchResults() == false) { @@ -58,9 +65,7 @@ protected void doCheckNoMissingShards(String phaseName, SearchRequest request, G } if (missingShards.isEmpty() == false) { // Status red - shard is missing all copies and would produce partial results for an index search - final String msg = "Search rejected due to missing shards [" - + missingShards - + "]. Consider using `allow_partial_search_results` setting to bypass this error."; + final String msg = missingShardsErrorMessage(missingShards); throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY); } } 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 59c348d751c5d..3186003cde6e4 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -214,27 +214,8 @@ SearchPhase openPointInTimePhase( clusters ) { @Override - protected void doCheckNoMissingShards( - String phaseName, - SearchRequest request, - GroupShardsIterator shardsIts - ) { - final StringBuilder missingShards = new StringBuilder(); - // Fail-fast verification of all shards being available - for (int index = 0; index < shardsIts.size(); index++) { - final SearchShardIterator shardRoutings = shardsIts.get(index); - if (shardRoutings.size() == 0) { - if (missingShards.isEmpty() == false) { - missingShards.append(", "); - } - missingShards.append(shardRoutings.shardId()); - } - } - if (missingShards.isEmpty() == false) { - // Status red - shard is missing all copies and would produce partial results for an index search - final String msg = "Cannot execute [open_point_in_time] action due to missing shards [" + missingShards + "]."; - throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY); - } + protected String missingShardsErrorMessage(StringBuilder missingShards) { + return "Cannot execute [open_point_in_time] action due to missing shards [" + missingShards + "]."; } @Override From 933d6cd572337b812960a24619ac2ff56b62773c Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Fri, 26 Jul 2024 08:25:54 +0300 Subject: [PATCH 4/6] iter --- .../main/java/org/elasticsearch/action/search/SearchPhase.java | 3 +-- .../action/search/TransportOpenPointInTimeAction.java | 2 +- .../xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java | 2 +- .../xpack/sql/qa/single_node/JdbcShardFailureIT.java | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) 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 cb940d29efdab..da8479873a4b6 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhase.java @@ -15,7 +15,6 @@ import java.io.IOException; import java.io.UncheckedIOException; -import java.util.List; import java.util.Objects; /** @@ -43,7 +42,7 @@ public void start() { } } - protected String missingShardsErrorMessage(StringBuilder missingShards){ + protected String missingShardsErrorMessage(StringBuilder missingShards) { return "Search rejected due to missing shards [" + missingShards + "]. Consider using `allow_partial_search_results` setting to bypass this error."; 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 3186003cde6e4..0494b18f62013 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -215,7 +215,7 @@ SearchPhase openPointInTimePhase( ) { @Override protected String missingShardsErrorMessage(StringBuilder missingShards) { - return "Cannot execute [open_point_in_time] action due to missing shards [" + missingShards + "]."; + return "Search rejected due to missing shards. [open_point_in_time] action requires all shards to be available."; } @Override 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 646750fe56d35..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("Cannot execute [open_point_in_time] action due to missing shards")); + assertThat(exception.getMessage(), containsString("Search rejected due to missing shards")); } } } 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 2a4b547f095ab..dc9989b26c3b2 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("Cannot execute [open_point_in_time] action due to missing shards")); + assertThat(exception.getMessage(), containsString("Search rejected due to missing shards")); } } From 94f94b3db9eb059de4d9386fdf0994558f437ad4 Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Mon, 29 Jul 2024 13:03:42 +0300 Subject: [PATCH 5/6] addressing PR comments - updating log message --- .../action/search/TransportOpenPointInTimeAction.java | 2 +- .../xpack/sql/qa/jdbc/single_node/JdbcShardFailureIT.java | 2 +- .../xpack/sql/qa/single_node/JdbcShardFailureIT.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 0494b18f62013..0e419f10153c3 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -215,7 +215,7 @@ SearchPhase openPointInTimePhase( ) { @Override protected String missingShardsErrorMessage(StringBuilder missingShards) { - return "Search rejected due to missing shards. [open_point_in_time] action requires all shards to be available."; + return "[open_point_in_time] action requires all shards to be available. Missing shards: " + missingShards; } @Override 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 f83047411f0b0..0e0f7dc9722d9 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("Search rejected due to missing shards")); + assertThat(exception.getMessage(), containsString("[open_point_in_time] action requires all shards to be available")); } } } 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 dc9989b26c3b2..91f3ab029f55c 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("Search rejected due to missing shards")); + assertThat(exception.getMessage(), containsString("[open_point_in_time] action requires all shards to be available")); } } From 1f30d217a330b87762551cb1a8c13817519d7651 Mon Sep 17 00:00:00 2001 From: Panagiotis Bailis Date: Mon, 29 Jul 2024 13:05:14 +0300 Subject: [PATCH 6/6] addressing PR comments - updating log message --- .../action/search/TransportOpenPointInTimeAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0e419f10153c3..92d90fa8e55ad 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -215,7 +215,7 @@ SearchPhase openPointInTimePhase( ) { @Override protected String missingShardsErrorMessage(StringBuilder missingShards) { - return "[open_point_in_time] action requires all shards to be available. Missing shards: " + missingShards; + return "[open_point_in_time] action requires all shards to be available. Missing shards: [" + missingShards + "]"; } @Override