Skip to content

Commit 50ddd7d

Browse files
javannajavanna
committed
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.
1 parent 4356671 commit 50ddd7d

File tree

6 files changed

+102
-32
lines changed

6 files changed

+102
-32
lines changed

‎server/src/main/java/org/elasticsearch/ElasticsearchException.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,6 @@ public String getDetailedMessage() {
269269
}
270270
}
271271

272-
273272
/**
274273
* Retrieve the innermost cause of this exception, if none, returns the current exception.
275274
*/
@@ -292,7 +291,7 @@ public void writeTo(StreamOutput out) throws IOException {
292291
out.writeMapOfLists(headers, StreamOutput::writeString, StreamOutput::writeString);
293292
out.writeMapOfLists(metadata, StreamOutput::writeString, StreamOutput::writeString);
294293
} else {
295-
HashMap<String, List<String>> finalHeaders = new HashMap<>(headers.size() + metadata.size());
294+
Map<String, List<String>> finalHeaders = new HashMap<>(headers.size() + metadata.size());
296295
finalHeaders.putAll(headers);
297296
finalHeaders.putAll(metadata);
298297
out.writeMapOfLists(finalHeaders, StreamOutput::writeString, StreamOutput::writeString);

‎server/src/main/java/org/elasticsearch/action/FailedNodeException.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.elasticsearch.ElasticsearchException;
2323
import org.elasticsearch.common.io.stream.StreamInput;
2424
import org.elasticsearch.common.io.stream.StreamOutput;
25+
import org.elasticsearch.common.xcontent.XContentBuilder;
2526

2627
import java.io.IOException;
2728

@@ -48,4 +49,9 @@ public void writeTo(StreamOutput out) throws IOException {
4849
super.writeTo(out);
4950
out.writeOptionalString(nodeId);
5051
}
52+
53+
@Override
54+
protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException {
55+
builder.field("node_id", nodeId);
56+
}
5157
}

‎server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,6 @@ public static ListTasksResponse fromXContent(XContentParser parser) {
266266

267267
@Override
268268
public String toString() {
269-
return Strings.toString(this);
269+
return Strings.toString(this, true, true);
270270
}
271271
}

‎server/src/main/java/org/elasticsearch/tasks/TaskInfo.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import org.elasticsearch.common.unit.TimeValue;
3030
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
3131
import org.elasticsearch.common.xcontent.ObjectParserHelper;
32-
import org.elasticsearch.common.xcontent.ToXContent.Params;
3332
import org.elasticsearch.common.xcontent.ToXContentFragment;
3433
import org.elasticsearch.common.xcontent.XContentBuilder;
3534
import org.elasticsearch.common.xcontent.XContentParser;
@@ -259,7 +258,7 @@ public static TaskInfo fromXContent(XContentParser parser) {
259258

260259
@Override
261260
public String toString() {
262-
return Strings.toString(this);
261+
return Strings.toString(this, true, true);
263262
}
264263

265264
// Implements equals and hashCode for testing

‎server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java‎

Lines changed: 91 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,78 +23,143 @@
2323
import org.elasticsearch.action.FailedNodeException;
2424
import org.elasticsearch.action.TaskOperationFailure;
2525
import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse;
26-
import org.elasticsearch.common.bytes.BytesReference;
27-
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
28-
import org.elasticsearch.common.xcontent.XContentFactory;
29-
import org.elasticsearch.common.xcontent.XContentHelper;
26+
import org.elasticsearch.common.Strings;
3027
import org.elasticsearch.common.xcontent.XContentParser;
31-
import org.elasticsearch.common.xcontent.XContentType;
3228
import org.elasticsearch.test.AbstractXContentTestCase;
3329

3430
import java.io.IOException;
31+
import java.net.ConnectException;
3532
import java.util.ArrayList;
3633
import java.util.Collections;
3734
import java.util.List;
38-
import java.util.Objects;
35+
import java.util.function.Predicate;
36+
import java.util.function.Supplier;
3937

4038
import static java.util.Collections.emptyList;
4139
import static java.util.Collections.singletonList;
4240
import static org.hamcrest.Matchers.equalTo;
43-
import static org.hamcrest.Matchers.notNullValue;
41+
import static org.hamcrest.Matchers.instanceOf;
4442

4543
public class ListTasksResponseTests extends AbstractXContentTestCase<ListTasksResponse> {
4644

4745
public void testEmptyToString() {
48-
assertEquals("{\"tasks\":[]}", new ListTasksResponse().toString());
46+
assertEquals("{\n" +
47+
" \"tasks\" : [ ]\n" +
48+
"}", new ListTasksResponse().toString());
4949
}
5050

5151
public void testNonEmptyToString() {
5252
TaskInfo info = new TaskInfo(
5353
new TaskId("node1", 1), "dummy-type", "dummy-action", "dummy-description", null, 0, 1, true, new TaskId("node1", 0),
5454
Collections.singletonMap("foo", "bar"));
5555
ListTasksResponse tasksResponse = new ListTasksResponse(singletonList(info), emptyList(), emptyList());
56-
assertEquals("{\"tasks\":[{\"node\":\"node1\",\"id\":1,\"type\":\"dummy-type\",\"action\":\"dummy-action\","
57-
+ "\"description\":\"dummy-description\",\"start_time_in_millis\":0,\"running_time_in_nanos\":1,\"cancellable\":true,"
58-
+ "\"parent_task_id\":\"node1:0\",\"headers\":{\"foo\":\"bar\"}}]}", tasksResponse.toString());
56+
assertEquals("{\n" +
57+
" \"tasks\" : [\n" +
58+
" {\n" +
59+
" \"node\" : \"node1\",\n" +
60+
" \"id\" : 1,\n" +
61+
" \"type\" : \"dummy-type\",\n" +
62+
" \"action\" : \"dummy-action\",\n" +
63+
" \"description\" : \"dummy-description\",\n" +
64+
" \"start_time\" : \"1970-01-01T00:00:00.000Z\",\n" +
65+
" \"start_time_in_millis\" : 0,\n" +
66+
" \"running_time\" : \"1nanos\",\n" +
67+
" \"running_time_in_nanos\" : 1,\n" +
68+
" \"cancellable\" : true,\n" +
69+
" \"parent_task_id\" : \"node1:0\",\n" +
70+
" \"headers\" : {\n" +
71+
" \"foo\" : \"bar\"\n" +
72+
" }\n" +
73+
" }\n" +
74+
" ]\n" +
75+
"}", tasksResponse.toString());
5976
}
6077

6178
@Override
6279
protected ListTasksResponse createTestInstance() {
80+
//failures are tested separately, so we can test xcontent equivalence at least when we have no failures
81+
return new ListTasksResponse(randomTasks(), Collections.emptyList(), Collections.emptyList());
82+
}
83+
84+
private static List<TaskInfo> randomTasks() {
6385
List<TaskInfo> tasks = new ArrayList<>();
6486
for (int i = 0; i < randomInt(10); i++) {
6587
tasks.add(TaskInfoTests.randomTaskInfo());
6688
}
67-
List<TaskOperationFailure> taskFailures = new ArrayList<>();
68-
for (int i = 0; i < randomInt(5); i++) {
69-
taskFailures.add(new TaskOperationFailure(
70-
randomAlphaOfLength(5), randomNonNegativeLong(), new IllegalStateException("message")));
71-
}
72-
return new ListTasksResponse(tasks, taskFailures, Collections.singletonList(new FailedNodeException("", "message", null)));
89+
return tasks;
7390
}
7491

7592
@Override
76-
protected ListTasksResponse doParseInstance(XContentParser parser) throws IOException {
93+
protected ListTasksResponse doParseInstance(XContentParser parser) {
7794
return ListTasksResponse.fromXContent(parser);
7895
}
7996

8097
@Override
8198
protected boolean supportsUnknownFields() {
82-
return false;
99+
return true;
100+
}
101+
102+
@Override
103+
protected Predicate<String> getRandomFieldsExcludeFilter() {
104+
//status and headers hold arbitrary content, we can't inject random fields in them
105+
return field -> field.endsWith("status") || field.endsWith("headers");
83106
}
84107

85108
@Override
86109
protected void assertEqualInstances(ListTasksResponse expectedInstance, ListTasksResponse newInstance) {
87110
assertNotSame(expectedInstance, newInstance);
88111
assertThat(newInstance.getTasks(), equalTo(expectedInstance.getTasks()));
89-
assertThat(newInstance.getNodeFailures().size(), equalTo(1));
90-
for (ElasticsearchException failure : newInstance.getNodeFailures()) {
91-
assertThat(failure, notNullValue());
92-
assertThat(failure.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=message]"));
112+
assertThat(newInstance.getNodeFailures().size(), equalTo(expectedInstance.getNodeFailures().size()));
113+
for (int i = 0; i < newInstance.getNodeFailures().size(); i++) {
114+
ElasticsearchException newException = newInstance.getNodeFailures().get(i);
115+
ElasticsearchException expectedException = expectedInstance.getNodeFailures().get(i);
116+
assertThat(newException.getMetadata("es.node_id").get(0), equalTo(((FailedNodeException)expectedException).nodeId()));
117+
assertThat(newException.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=error message]"));
118+
assertThat(newException.getCause(), instanceOf(ElasticsearchException.class));
119+
ElasticsearchException cause = (ElasticsearchException) newException.getCause();
120+
assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=connect_exception, reason=null]"));
121+
}
122+
assertThat(newInstance.getTaskFailures().size(), equalTo(expectedInstance.getTaskFailures().size()));
123+
for (int i = 0; i < newInstance.getTaskFailures().size(); i++) {
124+
TaskOperationFailure newFailure = newInstance.getTaskFailures().get(i);
125+
TaskOperationFailure expectedFailure = expectedInstance.getTaskFailures().get(i);
126+
assertThat(newFailure.getNodeId(), equalTo(expectedFailure.getNodeId()));
127+
assertThat(newFailure.getTaskId(), equalTo(expectedFailure.getTaskId()));
128+
assertThat(newFailure.getStatus(), equalTo(expectedFailure.getStatus()));
129+
assertThat(newFailure.getCause(), instanceOf(ElasticsearchException.class));
130+
ElasticsearchException cause = (ElasticsearchException) newFailure.getCause();
131+
assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=illegal_state_exception, reason=null]"));
93132
}
94133
}
95134

96-
@Override
97-
protected boolean assertToXContentEquivalence() {
98-
return false;
135+
/**
136+
* Test parsing {@link ListTasksResponse} with inner failures as they don't support asserting on xcontent equivalence, given that
137+
* exceptions are not parsed back as the same original class. We run the usual {@link AbstractXContentTestCase#testFromXContent()}
138+
* without failures, and this other test with failures where we disable asserting on xcontent equivalence at the end.
139+
*/
140+
public void testFromXContentWithFailures() throws IOException {
141+
Supplier<ListTasksResponse> instanceSupplier = ListTasksResponseTests::createTestInstanceWithFailures;
142+
//with random fields insertion in the inner exceptions, some random stuff may be parsed back as metadata,
143+
//but that does not bother our assertions, as we only want to test that we don't break.
144+
boolean supportsUnknownFields = true;
145+
//exceptions are not of the same type whenever parsed back
146+
boolean assertToXContentEquivalence = false;
147+
AbstractXContentTestCase.testFromXContent(NUMBER_OF_TEST_RUNS, instanceSupplier, supportsUnknownFields, Strings.EMPTY_ARRAY,
148+
getRandomFieldsExcludeFilter(), this::createParser, this::doParseInstance,
149+
this::assertEqualInstances, assertToXContentEquivalence);
150+
}
151+
152+
private static ListTasksResponse createTestInstanceWithFailures() {
153+
int numNodeFailures = randomIntBetween(0, 3);
154+
List<FailedNodeException> nodeFailures = new ArrayList<>(numNodeFailures);
155+
for (int i = 0; i < numNodeFailures; i++) {
156+
nodeFailures.add(new FailedNodeException(randomAlphaOfLength(5), "error message", new ConnectException()));
157+
}
158+
int numTaskFailures = randomIntBetween(0, 3);
159+
List<TaskOperationFailure> taskFailures = new ArrayList<>(numTaskFailures);
160+
for (int i = 0; i < numTaskFailures; i++) {
161+
taskFailures.add(new TaskOperationFailure(randomAlphaOfLength(5), randomLong(), new IllegalStateException()));
162+
}
163+
return new ListTasksResponse(randomTasks(), taskFailures, nodeFailures);
99164
}
100165
}

‎server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,12 @@ protected boolean supportsUnknownFields() {
6363

6464
@Override
6565
protected Predicate<String> getRandomFieldsExcludeFilter() {
66+
//status and headers hold arbitrary content, we can't inject random fields in them
6667
return field -> "status".equals(field) || "headers".equals(field);
6768
}
6869

6970
@Override
70-
protected TaskInfo mutateInstance(TaskInfo info) throws IOException {
71+
protected TaskInfo mutateInstance(TaskInfo info) {
7172
switch (between(0, 9)) {
7273
case 0:
7374
TaskId taskId = new TaskId(info.getTaskId().getNodeId() + randomAlphaOfLength(5), info.getTaskId().getId());

0 commit comments

Comments
 (0)