Reindex and friends fail on RED shards

Reindex, update by query and delete by query would silently disregard RED/unavailable shards, thus not copying, updating or deleting matching data in those shards. Now use `allow_partial_search_results=false` to ensure these operations fail if the search crosses an unavailable chard. Relates elastic#45739 and elastic#42612
henningandersen · Aug 22, 2019 · 3a5853e · 3a5853e
1 parent 31f6e78
commit 3a5853e
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 24 deletions.
diff --git a/...s/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java b/...s/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java
@@ -125,6 +125,11 @@ static Request initialSearch(SearchRequest searchRequest, BytesReference query,
             request.addParameter(storedFieldsParamName, fields.toString());
         }
 
+        if (remoteVersion.onOrAfter(Version.fromId(6030099))) {
+            // allow_partial_results introduced in 6.3, running remote reindex against earlier versions still silently discards RED shards.
+            request.addParameter("allow_partial_search_results", "false");
+        }
+
         // EMPTY is safe here because we're not calling namedObject
         try (XContentBuilder entity = JsonXContent.contentBuilder();
                 XContentParser queryParser = XContentHelper

diff --git a/...reindex/src/test/java/org/elasticsearch/index/reindex/ClientScrollableHitSourceTests.java b/...reindex/src/test/java/org/elasticsearch/index/reindex/ClientScrollableHitSourceTests.java
@@ -117,7 +117,7 @@ private void dotestBasicsWithRetry(int retries, int minFailures, int maxFailures
             client.awaitOperation();
             ++expectedSearchRetries;
         }
-
+        client.validateRequest(SearchAction.INSTANCE, (SearchRequest r) -> assertTrue(r.allowPartialSearchResults() == Boolean.FALSE));
         SearchResponse searchResponse = createSearchResponse();
         client.respond(SearchAction.INSTANCE, searchResponse);
 

diff --git a/...ndex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java b/...ndex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java
@@ -40,6 +40,7 @@
 import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.clearScroll;
 import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.initialSearch;
 import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.scroll;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.either;
 import static org.hamcrest.Matchers.empty;
@@ -178,6 +179,22 @@ public void testInitialSearchParamsMisc() {
         }
     }
 
+    public void testInitialSearchDisallowPartialResults() {
+        final String allowPartialParamName = "allow_partial_search_results";
+        final int v6_3 = 6030099;
+
+        BytesReference query = new BytesArray("{}");
+        SearchRequest searchRequest = new SearchRequest().source(new SearchSourceBuilder());
+
+        Version disallowVersion = Version.fromId(between(v6_3, Version.CURRENT.id));
+        Map<String, String> params = initialSearch(searchRequest, query, disallowVersion).getParameters();
+        assertEquals("false", params.get(allowPartialParamName));
+
+        Version allowVersion = Version.fromId(between(0, v6_3-1));
+        params = initialSearch(searchRequest, query, allowVersion).getParameters();
+        assertThat(params.keySet(), not(contains(allowPartialParamName)));
+    }
+
     private void assertScroll(Version remoteVersion, Map<String, String> params, TimeValue requested) {
         // V_5_0_0
         if (remoteVersion.before(Version.fromId(5000099))) {

diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml
@@ -28,15 +28,15 @@
                   source: throw new IllegalArgumentException("Cats!")
           dest:
             index:   dest
-  - match: {created: 0}
-  - match: {updated: 0}
-  - match: {version_conflicts: 0}
-  - match: {batches: 0}
-  - match: {failures.0.shard: 0}
-  - match: {failures.0.index:  source}
-  - is_true: failures.0.node
-  - match: {failures.0.reason.type:   script_exception}
-  - match: {failures.0.reason.reason: runtime error}
-  - match: {failures.0.reason.caused_by.type:   illegal_argument_exception}
-  - match: {failures.0.reason.caused_by.reason: Cats!}
-  - gte: { took: 0 }
+  - match: {error.type: search_phase_execution_exception}
+  - match: {error.reason: "Partial shards failure"}
+  - match: {error.phase: query}
+  - match: {error.root_cause.0.type:   script_exception}
+  - match: {error.root_cause.0.reason: runtime error}
+  - match: {error.failed_shards.0.shard: 0}
+  - match: {error.failed_shards.0.index:  source}
+  - is_true: error.failed_shards.0.node
+  - match: {error.failed_shards.0.reason.type:   script_exception}
+  - match: {error.failed_shards.0.reason.reason: runtime error}
+  - match: {error.failed_shards.0.reason.caused_by.type:   illegal_argument_exception}
+  - match: {error.failed_shards.0.reason.caused_by.reason: Cats!}
diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml
@@ -25,14 +25,15 @@
               script:
                 lang: painless
                 source: throw new IllegalArgumentException("Cats!")
-  - match: {updated: 0}
-  - match: {version_conflicts: 0}
-  - match: {batches: 0}
-  - match: {failures.0.shard: 0}
-  - match: {failures.0.index:  source}
-  - is_true: failures.0.node
-  - match: {failures.0.reason.type:   script_exception}
-  - match: {failures.0.reason.reason: runtime error}
-  - match: {failures.0.reason.caused_by.type:   illegal_argument_exception}
-  - match: {failures.0.reason.caused_by.reason: Cats!}
-  - gte: { took: 0 }
+  - match: {error.type: search_phase_execution_exception}
+  - match: {error.reason: "Partial shards failure"}
+  - match: {error.phase: query}
+  - match: {error.root_cause.0.type:   script_exception}
+  - match: {error.root_cause.0.reason: runtime error}
+  - match: {error.failed_shards.0.shard: 0}
+  - match: {error.failed_shards.0.index:  source}
+  - is_true: error.failed_shards.0.node
+  - match: {error.failed_shards.0.reason.type:   script_exception}
+  - match: {error.failed_shards.0.reason.reason: runtime error}
+  - match: {error.failed_shards.0.reason.caused_by.type:   illegal_argument_exception}
+  - match: {error.failed_shards.0.reason.caused_by.reason: Cats!}
diff --git a/server/src/main/java/org/elasticsearch/index/reindex/ClientScrollableHitSource.java b/server/src/main/java/org/elasticsearch/index/reindex/ClientScrollableHitSource.java
@@ -64,6 +64,7 @@ public ClientScrollableHitSource(Logger logger, BackoffPolicy backoffPolicy, Thr
         super(logger, backoffPolicy, threadPool, countSearchRetry, onResponse, fail);
         this.client = client;
         this.firstSearchRequest = firstSearchRequest;
+        firstSearchRequest.allowPartialSearchResults(false);
     }
 
     @Override