From 31351ab88036d8387f628030eb7f9c48070fef0f Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Fri, 1 Jun 2018 08:53:24 +0200 Subject: [PATCH] High-level client: list tasks failure to not lose nodeId (#31001) This commit reworks testing for `ListTasksResponse` so that random fields insertion can be tested and xcontent equivalence can be checked too. Proper exclusions need to be configured, and failures need to be tested separately. This helped finding a little problem, whenever there is a node failure returned, the nodeId was lost as it was never printed out as part of the exception toXContent. --- .../elasticsearch/ElasticsearchException.java | 3 +- .../action/FailedNodeException.java | 6 + .../node/tasks/list/ListTasksResponse.java | 2 +- .../org/elasticsearch/tasks/TaskInfo.java | 3 +- .../tasks/ListTasksResponseTests.java | 117 ++++++++++++++---- .../elasticsearch/tasks/TaskInfoTests.java | 3 +- 6 files changed, 102 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ElasticsearchException.java b/server/src/main/java/org/elasticsearch/ElasticsearchException.java index db8263d1513..db26cb2a5e9 100644 --- a/server/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/server/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -269,7 +269,6 @@ public class ElasticsearchException extends RuntimeException implements ToXConte } } - /** * Retrieve the innermost cause of this exception, if none, returns the current exception. */ @@ -292,7 +291,7 @@ public class ElasticsearchException extends RuntimeException implements ToXConte out.writeMapOfLists(headers, StreamOutput::writeString, StreamOutput::writeString); out.writeMapOfLists(metadata, StreamOutput::writeString, StreamOutput::writeString); } else { - HashMap> finalHeaders = new HashMap<>(headers.size() + metadata.size()); + Map> finalHeaders = new HashMap<>(headers.size() + metadata.size()); finalHeaders.putAll(headers); finalHeaders.putAll(metadata); out.writeMapOfLists(finalHeaders, StreamOutput::writeString, StreamOutput::writeString); diff --git a/server/src/main/java/org/elasticsearch/action/FailedNodeException.java b/server/src/main/java/org/elasticsearch/action/FailedNodeException.java index bf9aad0d39e..475de7eae5f 100644 --- a/server/src/main/java/org/elasticsearch/action/FailedNodeException.java +++ b/server/src/main/java/org/elasticsearch/action/FailedNodeException.java @@ -22,6 +22,7 @@ package org.elasticsearch.action; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; @@ -48,4 +49,9 @@ public class FailedNodeException extends ElasticsearchException { super.writeTo(out); out.writeOptionalString(nodeId); } + + @Override + protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException { + builder.field("node_id", nodeId); + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java index 1233b7143ab..53d80853328 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java @@ -266,6 +266,6 @@ public class ListTasksResponse extends BaseTasksResponse implements ToXContentOb @Override public String toString() { - return Strings.toString(this); + return Strings.toString(this, true, true); } } diff --git a/server/src/main/java/org/elasticsearch/tasks/TaskInfo.java b/server/src/main/java/org/elasticsearch/tasks/TaskInfo.java index 26aabec3e9f..d01b8cfaa3c 100644 --- a/server/src/main/java/org/elasticsearch/tasks/TaskInfo.java +++ b/server/src/main/java/org/elasticsearch/tasks/TaskInfo.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParserHelper; -import org.elasticsearch.common.xcontent.ToXContent.Params; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -259,7 +258,7 @@ public final class TaskInfo implements Writeable, ToXContentFragment { @Override public String toString() { - return Strings.toString(this); + return Strings.toString(this, true, true); } // Implements equals and hashCode for testing diff --git a/server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java b/server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java index 295ff955e41..b280446db1c 100644 --- a/server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java +++ b/server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java @@ -23,29 +23,29 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.TaskOperationFailure; import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse; -import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.AbstractXContentTestCase; import java.io.IOException; +import java.net.ConnectException; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Objects; +import java.util.function.Predicate; +import java.util.function.Supplier; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.instanceOf; public class ListTasksResponseTests extends AbstractXContentTestCase { public void testEmptyToString() { - assertEquals("{\"tasks\":[]}", new ListTasksResponse().toString()); + assertEquals("{\n" + + " \"tasks\" : [ ]\n" + + "}", new ListTasksResponse().toString()); } public void testNonEmptyToString() { @@ -53,48 +53,113 @@ public class ListTasksResponseTests extends AbstractXContentTestCase randomTasks() { List tasks = new ArrayList<>(); for (int i = 0; i < randomInt(10); i++) { tasks.add(TaskInfoTests.randomTaskInfo()); } - List taskFailures = new ArrayList<>(); - for (int i = 0; i < randomInt(5); i++) { - taskFailures.add(new TaskOperationFailure( - randomAlphaOfLength(5), randomNonNegativeLong(), new IllegalStateException("message"))); - } - return new ListTasksResponse(tasks, taskFailures, Collections.singletonList(new FailedNodeException("", "message", null))); + return tasks; } @Override - protected ListTasksResponse doParseInstance(XContentParser parser) throws IOException { + protected ListTasksResponse doParseInstance(XContentParser parser) { return ListTasksResponse.fromXContent(parser); } @Override protected boolean supportsUnknownFields() { - return false; + return true; + } + + @Override + protected Predicate getRandomFieldsExcludeFilter() { + //status and headers hold arbitrary content, we can't inject random fields in them + return field -> field.endsWith("status") || field.endsWith("headers"); } @Override protected void assertEqualInstances(ListTasksResponse expectedInstance, ListTasksResponse newInstance) { assertNotSame(expectedInstance, newInstance); assertThat(newInstance.getTasks(), equalTo(expectedInstance.getTasks())); - assertThat(newInstance.getNodeFailures().size(), equalTo(1)); - for (ElasticsearchException failure : newInstance.getNodeFailures()) { - assertThat(failure, notNullValue()); - assertThat(failure.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=message]")); + assertThat(newInstance.getNodeFailures().size(), equalTo(expectedInstance.getNodeFailures().size())); + for (int i = 0; i < newInstance.getNodeFailures().size(); i++) { + ElasticsearchException newException = newInstance.getNodeFailures().get(i); + ElasticsearchException expectedException = expectedInstance.getNodeFailures().get(i); + assertThat(newException.getMetadata("es.node_id").get(0), equalTo(((FailedNodeException)expectedException).nodeId())); + assertThat(newException.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=error message]")); + assertThat(newException.getCause(), instanceOf(ElasticsearchException.class)); + ElasticsearchException cause = (ElasticsearchException) newException.getCause(); + assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=connect_exception, reason=null]")); + } + assertThat(newInstance.getTaskFailures().size(), equalTo(expectedInstance.getTaskFailures().size())); + for (int i = 0; i < newInstance.getTaskFailures().size(); i++) { + TaskOperationFailure newFailure = newInstance.getTaskFailures().get(i); + TaskOperationFailure expectedFailure = expectedInstance.getTaskFailures().get(i); + assertThat(newFailure.getNodeId(), equalTo(expectedFailure.getNodeId())); + assertThat(newFailure.getTaskId(), equalTo(expectedFailure.getTaskId())); + assertThat(newFailure.getStatus(), equalTo(expectedFailure.getStatus())); + assertThat(newFailure.getCause(), instanceOf(ElasticsearchException.class)); + ElasticsearchException cause = (ElasticsearchException) newFailure.getCause(); + assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=illegal_state_exception, reason=null]")); } } - @Override - protected boolean assertToXContentEquivalence() { - return false; + /** + * Test parsing {@link ListTasksResponse} with inner failures as they don't support asserting on xcontent equivalence, given that + * exceptions are not parsed back as the same original class. We run the usual {@link AbstractXContentTestCase#testFromXContent()} + * without failures, and this other test with failures where we disable asserting on xcontent equivalence at the end. + */ + public void testFromXContentWithFailures() throws IOException { + Supplier instanceSupplier = ListTasksResponseTests::createTestInstanceWithFailures; + //with random fields insertion in the inner exceptions, some random stuff may be parsed back as metadata, + //but that does not bother our assertions, as we only want to test that we don't break. + boolean supportsUnknownFields = true; + //exceptions are not of the same type whenever parsed back + boolean assertToXContentEquivalence = false; + AbstractXContentTestCase.testFromXContent(NUMBER_OF_TEST_RUNS, instanceSupplier, supportsUnknownFields, Strings.EMPTY_ARRAY, + getRandomFieldsExcludeFilter(), this::createParser, this::doParseInstance, + this::assertEqualInstances, assertToXContentEquivalence); + } + + private static ListTasksResponse createTestInstanceWithFailures() { + int numNodeFailures = randomIntBetween(0, 3); + List nodeFailures = new ArrayList<>(numNodeFailures); + for (int i = 0; i < numNodeFailures; i++) { + nodeFailures.add(new FailedNodeException(randomAlphaOfLength(5), "error message", new ConnectException())); + } + int numTaskFailures = randomIntBetween(0, 3); + List taskFailures = new ArrayList<>(numTaskFailures); + for (int i = 0; i < numTaskFailures; i++) { + taskFailures.add(new TaskOperationFailure(randomAlphaOfLength(5), randomLong(), new IllegalStateException())); + } + return new ListTasksResponse(randomTasks(), taskFailures, nodeFailures); } } diff --git a/server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java b/server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java index 616ac105387..bc57109802a 100644 --- a/server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java +++ b/server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java @@ -63,11 +63,12 @@ public class TaskInfoTests extends AbstractSerializingTestCase { @Override protected Predicate getRandomFieldsExcludeFilter() { + //status and headers hold arbitrary content, we can't inject random fields in them return field -> "status".equals(field) || "headers".equals(field); } @Override - protected TaskInfo mutateInstance(TaskInfo info) throws IOException { + protected TaskInfo mutateInstance(TaskInfo info) { switch (between(0, 9)) { case 0: TaskId taskId = new TaskId(info.getTaskId().getNodeId() + randomAlphaOfLength(5), info.getTaskId().getId());