From cd5c6222291cca9f2252c4b975f48c0852524b0a Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 28 Aug 2018 18:09:29 +0200 Subject: [PATCH] Remove unsupported group_shard_failures parameter We have had support for the `group_shard_failures` parameter in our code for a while, since we introduced failures grouping. When we introduced validation of parameters at REST, we seem to have forgotten to expose such parameter. Given that the parameter is effectively not supported for many months now, that no user has complained about that and that grouping is the expected behaviour, this commit removes support for the parameter. --- .../search/SearchPhaseExecutionException.java | 5 +- .../rest/action/RestActions.java | 3 +- .../SearchPhaseExecutionExceptionTests.java | 53 ------------------- .../AbstractBroadcastResponseTestCase.java | 24 --------- 4 files changed, 3 insertions(+), 82 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java b/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java index 3d3737b0638cd..fa532777e9dce 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java @@ -134,11 +134,10 @@ private static String buildMessage(String phaseName, String msg, ShardSearchFail @Override protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException { builder.field("phase", phaseName); - final boolean group = params.paramAsBoolean("group_shard_failures", true); // we group by default - builder.field("grouped", group); // notify that it's grouped + builder.field("grouped", true); // notify that it's grouped builder.field("failed_shards"); builder.startArray(); - ShardOperationFailedException[] failures = group ? ExceptionsHelper.groupBy(shardFailures) : shardFailures; + ShardOperationFailedException[] failures = ExceptionsHelper.groupBy(shardFailures); for (ShardOperationFailedException failure : failures) { builder.startObject(); failure.toXContent(builder, params); diff --git a/server/src/main/java/org/elasticsearch/rest/action/RestActions.java b/server/src/main/java/org/elasticsearch/rest/action/RestActions.java index 759cd4a773dd9..f25fd107e51db 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/RestActions.java +++ b/server/src/main/java/org/elasticsearch/rest/action/RestActions.java @@ -90,8 +90,7 @@ public static void buildBroadcastShardsHeader(XContentBuilder builder, Params pa builder.field(FAILED_FIELD.getPreferredName(), failed); if (shardFailures != null && shardFailures.length > 0) { builder.startArray(FAILURES_FIELD.getPreferredName()); - final boolean group = params.paramAsBoolean("group_shard_failures", true); // we group by default - for (ShardOperationFailedException shardFailure : group ? ExceptionsHelper.groupBy(shardFailures) : shardFailures) { + for (ShardOperationFailedException shardFailure : ExceptionsHelper.groupBy(shardFailures)) { builder.startObject(); shardFailure.toXContent(builder, params); builder.endObject(); diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchPhaseExecutionExceptionTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchPhaseExecutionExceptionTests.java index e96a0975fd46c..9fbf3704fff21 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchPhaseExecutionExceptionTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchPhaseExecutionExceptionTests.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContent; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; @@ -38,8 +37,6 @@ import java.io.IOException; -import static java.util.Collections.singletonMap; -import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.Matchers.hasSize; @@ -87,56 +84,6 @@ public void testToXContent() throws IOException { "}" + "}" + "]}", Strings.toString(exception)); - - // Failures are NOT grouped - ToXContent.MapParams params = new ToXContent.MapParams(singletonMap("group_shard_failures", "false")); - try (XContentBuilder builder = jsonBuilder()) { - builder.startObject(); - exception.toXContent(builder, params); - builder.endObject(); - - assertEquals("{" + - "\"type\":\"search_phase_execution_exception\"," + - "\"reason\":\"all shards failed\"," + - "\"phase\":\"test\"," + - "\"grouped\":false," + - "\"failed_shards\":[" + - "{" + - "\"shard\":0," + - "\"index\":\"foo\"," + - "\"node\":\"node_1\"," + - "\"reason\":{" + - "\"type\":\"parsing_exception\"," + - "\"reason\":\"foobar\"," + - "\"line\":1," + - "\"col\":2" + - "}" + - "}," + - "{" + - "\"shard\":1," + - "\"index\":\"foo\"," + - "\"node\":\"node_2\"," + - "\"reason\":{" + - "\"type\":\"index_shard_closed_exception\"," + - "\"reason\":\"CurrentState[CLOSED] Closed\"," + - "\"index_uuid\":\"_na_\"," + - "\"shard\":\"1\"," + - "\"index\":\"foo\"" + - "}" + - "}," + - "{" + - "\"shard\":2," + - "\"index\":\"foo\"," + - "\"node\":\"node_3\"," + - "\"reason\":{" + - "\"type\":\"parsing_exception\"," + - "\"reason\":\"foobar\"," + - "\"line\":5," + - "\"col\":7" + - "}" + - "}" + - "]}", Strings.toString(builder)); - } } public void testToAndFromXContent() throws IOException { diff --git a/server/src/test/java/org/elasticsearch/action/support/broadcast/AbstractBroadcastResponseTestCase.java b/server/src/test/java/org/elasticsearch/action/support/broadcast/AbstractBroadcastResponseTestCase.java index cec5e27f0763e..5bf48fa589764 100644 --- a/server/src/test/java/org/elasticsearch/action/support/broadcast/AbstractBroadcastResponseTestCase.java +++ b/server/src/test/java/org/elasticsearch/action/support/broadcast/AbstractBroadcastResponseTestCase.java @@ -33,7 +33,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import static org.hamcrest.CoreMatchers.anyOf; @@ -130,29 +129,6 @@ public void testFailuresDeduplication() throws IOException { assertThat(parsedFailures[1].shardId(), equalTo(2)); assertThat(parsedFailures[1].status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); assertThat(parsedFailures[1].getCause().getMessage(), containsString("fizz")); - - ToXContent.Params params = new ToXContent.MapParams(Collections.singletonMap("group_shard_failures", "false")); - BytesReference bytesReferenceWithoutDedup = toShuffledXContent(response, xContentType, params, humanReadable); - try(XContentParser parser = createParser(xContentType.xContent(), bytesReferenceWithoutDedup)) { - parsedResponse = doParseInstance(parser); - assertNull(parser.nextToken()); - } - - assertThat(parsedResponse.getShardFailures().length, equalTo(3)); - parsedFailures = parsedResponse.getShardFailures(); - for (int i = 0; i < 3; i++) { - if (i < 2) { - assertThat(parsedFailures[i].index(), equalTo("test")); - assertThat(parsedFailures[i].shardId(), equalTo(i)); - assertThat(parsedFailures[i].status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); - assertThat(parsedFailures[i].getCause().getMessage(), containsString("foo")); - } else { - assertThat(parsedFailures[i].index(), equalTo("test")); - assertThat(parsedFailures[i].shardId(), equalTo(i)); - assertThat(parsedFailures[i].status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); - assertThat(parsedFailures[i].getCause().getMessage(), containsString("fizz")); - } - } } public void testToXContent() {