Fix HLRC parsing of CancelTasks response #47017

jesinity · 2019-09-24T13:44:30Z

Some comments:

I created all client side counterparts for all server side objects involved in this exchange.
I did it also for ElasticSearchException and TaskOperationFailure. These are shallow copies of server side counterparts: at first I thought to create some common interfaces in the core module but then I reverted it back.
I did this way in order to cut down dependencies from server side classes, as I saw there are long term requirements to achieve it.
I did not manage to create complex statuses with failures or exceptions, but I went through how the response is generated server side.
I did not touch yet the ListTasksResponse to the other calls of the tasks api are still using old classes.
I did some unit tests I did not commit yet: some parsing tests are enough for the testing purposes?
I left out the parsing of the status field, which format changes very much over responses and is issued only if detailed parameter is issued to server.
I did it because I did not find an easy why doing it without falling back again on server side dependencies.
Status is converted to a raw format wrapping a BytesReference that exposes a Map<String,Object> functionality: I wonder if it is really something worth to have for the client.
I tried to set thing apart but at least a dependency on Lucene
org.apache.lucene.util.BytesRef in org.elasticsearch.common.bytes.BytesArray is unavoidable.
If you want I can add back the dependency on ObjectParserHelper, causing a dependency on server side libraries, or address me some other way around to fix the issue.

ghost · 2019-09-24T13:44:36Z

Hi @jesinity, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

elasticmachine · 2019-09-24T14:07:03Z

Pinging @elastic/es-core-features

hub-cap · 2019-09-25T16:58:13Z

Thanks for the submission! Ill review this soon!

hub-cap · 2019-10-09T17:20:27Z

ok finally getting my head above water, will be reviewing today!

jesinity · 2019-10-10T05:31:52Z

Amazing!

hub-cap · 2019-10-10T21:48:00Z

hey @jesinity I have begun reviewing the code. One thing I would like to point out is that I think the response class hierarchy is too heavy for the client. I would rather see something (this is only the Task/Node Exceptions, but I will be able to provide more info tomorrow) like the following.

What you will notice is that there is a response in the test case (i only did this for example purposes). This response contains the 2 lists of exceptions that can be returned, but if you notice, its only a nested map, rather than a full blown client side Exception hierarchy. Since these exceptions are generic due to the fact they are parsed from json, it does not make much sense to build real exceptions from them. They arent going to be throwable in code, so the normal things you would do with them in a java ecosystem dont apply. This being the case, the best you can do is inspect the strings within the class, so it makes sense to just keep it a generic map and let the user do what they will with it. I think there are further things we can do with the other parts of the response, but I need a bit more time to look thru the code. I will provide a better explanation tomorrow (hopefully). It is a day off for my kids and well, kids are crazzzzzzzy sometimes in the house :p

One other thing to note is that the TaskInfo can and should be a generic string. A client would never build one of those, because they are always generated from elasticsearch anyway, and the task code should always be using a string instead of a TaskId. This may not be the case for other client methods, but its the right way to go here. More to come soon!

Also I want to say thank you very much for the contribution. This is a doozy of a split between client/server, and I am very happy you took it on. We will get thru it together :D

There is a newer copy of this code below

#47017 (comment)

package org.elasticsearch.client.core;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.TaskOperationFailure;
import org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse;
import org.elasticsearch.client.AbstractResponseTestCase;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.tasks.TaskInfo;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

public class CancelTasksResponseTests extends AbstractResponseTestCase<CancelTasksResponse, CancelTasksResponseTests.CancelTasksResponse> {

    @Override
    protected org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse createServerTestInstance(XContentType xContentType) {
        List<TaskInfo> tasks = new ArrayList<>();
        List<TaskOperationFailure> taskFailures = new ArrayList<>();
        List<ElasticsearchException> nodeFailures = new ArrayList<>();

        for (int i = 0; i < randomIntBetween(1, 4); i++) {
            taskFailures.add(new TaskOperationFailure(randomAlphaOfLength(4), randomLongBetween(1, 3),
                new RuntimeException(randomAlphaOfLength(4))));
        }
        for (int i = 0; i < randomIntBetween(1, 4); i++) {
            nodeFailures.add(new ElasticsearchException(new RuntimeException(randomAlphaOfLength(10))));
        }

        return new org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse(tasks, taskFailures, nodeFailures);
    }

    @Override
    protected CancelTasksResponseTests.CancelTasksResponse doParseToClientInstance(XContentParser parser) throws IOException {
        return CancelTasksResponse.fromXContent(parser);
    }

    @Override
    protected void assertInstances(org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse serverTestInstance,
                                   CancelTasksResponseTests.CancelTasksResponse clientInstance) {

    }

    public static class CancelTasksResponse {
        private static ConstructingObjectParser<CancelTasksResponse, Void> PARSER  = new ConstructingObjectParser<>("cancel_tasks_response", true,
        constructingObjects -> {
            List<Map<String, Object>> tasksFailures = (List<Map<String, Object>>) constructingObjects[0];
            List<Map<String, Object>> nodeFailures = (List<Map<String, Object>>) constructingObjects[1];

            return new CancelTasksResponse(tasksFailures, nodeFailures);
        });

        static {
            PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> p.map(), new ParseField("task_failures"));
            PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> p.map(), new ParseField("node_failures"));
        }

        private List<Map<String, Object>> nodeFailures;
        private List<Map<String, Object>> taskFailures;

        public CancelTasksResponse(List<Map<String, Object>> taskFailures, List<Map<String, Object>> nodeFailures) {
            this.taskFailures = taskFailures;
            this.nodeFailures = nodeFailures;
        }

        public static CancelTasksResponse fromXContent(XContentParser parser) throws IOException {
            return PARSER.parse(parser, null);
        }
    }
}

jesinity · 2019-10-11T12:51:42Z

ok, thanks for your comments, I agree with them, I think a shallow version of Exception is something more handy for the clients to use.
I'll be waiting for more comments to move it forward.
Also please check that Status stuff, that is parsed in in BytesReference.
Do you think it is also the case to add more tests to validate the JSON parsing of responses?

hub-cap · 2019-10-11T14:09:00Z

Do you think it is also the case to add more tests to validate the JSON parsing of responses?

Yes absolutely. This is why I put the example in a test, to also give u an example for how to do the tests for these classes.

Once I get in to the office Ill be doing my best to finish this review and get you to a place where you can begin working on it again.

hub-cap · 2019-10-11T15:15:52Z

re: the status stuff, I am not sure how much more we can do there currently besides just give it back to the user as a blob of bytes, just because it can vary wildly from task to task as per the javadocs :)

I think we might be in a place where eventually we can make concrete client side status classes with a contract but unless you need a particular status but I am perfectly fine just keeping it ~~raw~~ a map for now.

I think that (along with turning the TaskID into a string) will make the response easy. Also, there is a ton of extra code on the original response in server that is used by the cat api, and we really dont need to port that. I think the response above with the 3 things 1) a client side TaskInfo, 2) a List of task errors, 3) a List of node errors will get us the best, smallest, class.

hub-cap · 2019-10-11T15:25:17Z

10 minutes ago @hub-cap, you are just wrong.

instead of the status being bytes, lets also turn it into a nested map. This will give users the ability to inspect it without converting it to something first.

So basically in the parser you write, use the same .map() for the status field.

hub-cap

ok I think this will be great moving forward. thank you for the effort, I knew it would be a pesky API! :)

Feel free to ask me any Qs you might have regarding the comments!

hub-cap · 2019-10-09T17:22:29Z

client/rest-high-level/src/main/java/org/elasticsearch/client/TasksClient.java

-     * @return the response
-     * @throws IOException in case there is a problem sending the request or parsing back the response
-     *
-     */


I think its best to keep the javadoc in here

sorry I did not notice I removed those.

sorry I did not notice I removed those

hub-cap · 2019-10-09T17:22:44Z

client/rest-high-level/src/main/java/org/elasticsearch/client/TasksClient.java

@@ -135,6 +125,7 @@ public CancelTasksResponse cancel(CancelTasksRequest cancelTasksRequest, Request
     */
    public Cancellable cancelAsync(CancelTasksRequest cancelTasksRequest, RequestOptions options,
                                   ActionListener<CancelTasksResponse> listener) {
+


empty space is not needed

hub-cap · 2019-10-09T17:30:02Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksRequest.java

+    public static final String[] EMPTY_ARRAY = new String[0];
+    public static final String[] ALL_ACTIONS = EMPTY_ARRAY;
+    public static final String[] ALL_NODES = EMPTY_ARRAY;
+    private String[] nodes = ALL_NODES;


This can be simplified. In the HLRC its fine to just hold a list of nodes initialized by List.of() and we likely dont need those other String arrays above

hub-cap · 2019-10-09T17:30:16Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksRequest.java

+    public static final String[] ALL_NODES = EMPTY_ARRAY;
+    private String[] nodes = ALL_NODES;
+    private TimeValue timeout;
+    private String[] actions = ALL_ACTIONS;


Same with this above stuff.

hub-cap · 2019-10-09T17:30:40Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksRequest.java

+    private TimeValue timeout;
+    private String[] actions = ALL_ACTIONS;
+    private TaskId parentTaskId = TaskId.EMPTY_TASK_ID;
+    private TaskId taskId = TaskId.EMPTY_TASK_ID;


These two should be strings instead of a TaskId

consider that the TaskId is basically a wrapper of nodeId:taskId where I can appy wildcards.
Removing it is ok for me, but how would we go about passing these parameters and ensuring

wildcards are applied

emptiness is guarded
?
Shall I implement the logic in the request itself?

Also the isSet method is used to check the TaskId is set.
So overall I think it would be better to keep this class, as the burden to create it is not that big.

hub-cap · 2019-10-11T15:20:06Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksResponse.java

+        this.taskGroups.addAll(buildTaskGroups());
+    }
+
+    private List<TaskGroup> buildTaskGroups() {


I think all of this, including the classes you have created for this can be removed. This was only used in the CAT api on the server side, so we can just return the same thing the transport response returns.

https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/java-rest-high-cluster-cancel-tasks.html

they are present in the API, that why I kept them. BTW, here it uses the nodeId contained in the TaskId .

You are correct, I missed that this is all in the parent task to the CancelTasksResponse 👍

I think its worth moving them to a "list task response" common class tho like in server side, so we dont have to move them (or duplicate them cuz maybe the author did not see them here).

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/TaskId.java

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/TaskGroup.java

hub-cap · 2019-10-11T15:21:34Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/NodesInfoData.java

+/**
+ * Holder of data created out of a {@link CancelTasksResponse}
+ */
+public class NodesInfoData {


Im not sure we need any of this here. This is part of the cat api stuff mentioned above.

Well, these classes hold the tasks themselves (among other things) that is why they are there.
I think that they can be made package access so that the user does not even know they are there.

hub-cap · 2019-10-11T15:23:23Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/NodeData.java

+import org.elasticsearch.common.xcontent.ObjectParser;
+import org.elasticsearch.common.xcontent.XContentParser;
+
+public class NodeData {


We might eventually need this for other APIs but I think the TaskInfo class will suffice for the CancelTaskResponse

jesinity · 2019-10-20T09:32:24Z

I am implementing the changes and I have some comments/questions, see above

10 minutes ago @hub-cap, you are just wrong.

instead of the status being bytes, lets also turn it into a nested map. This will give users the ability to inspect it without converting it to something first.

So basically in the parser you write, use the same .map() for the status field.

so you mean that the status is going to be a nested
Map<String,Object> where the value is either

a value (String, Integer etc)
another Map<String,Object> ( a kind of Json object)
an array of Objects that can be values, or Map<String,Object>s

it is a bit daunting to implement but I think it is feasible.
I searched through the code and did not find anything for the purpose, so I think I have to code it.
Is it something we can postpone to some other ticket of does it need to be solved in this?

hub-cap · 2019-10-21T15:10:07Z

Hey @jesinity I dont want you to work too hard on an implementation there. The built in object parsers have a .map() method which turns everything under a key to a nested map. Let me update the example above so you can see what I am talking about, but it will be a few hours.

hub-cap · 2019-10-21T15:15:15Z

Also, re: the classes I mentioned removing, those are specific to the rest layer, and the rest calls (curl style) will return a different set of items than just the transport client. The transport client is here, org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse and it is what I was mapping out. In this commit I dont think we should be doing anything more complex than just mimicing that response, client side.

Most of that code is used here in the rest layer.

hub-cap · 2019-10-21T16:57:48Z

ok, the following snippet of code now has the TaskInfo class added in as well, which will give you an idea of how to turn the Status into a map. I certainly did not want you to spin wheels on this for a long time, these parsers can be very pesky!!

edit:
There are still some unknowns for you to solve, as well as lots of testing work to be done, with some real integration tests (ITTests) and I think this below will get you all of the server<->client parsing you will need to worry about.

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.TaskOperationFailure;
import org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse;
import org.elasticsearch.client.AbstractResponseTestCase;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.tasks.TaskId;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

public class CancelTaskResponseTests extends AbstractResponseTestCase<CancelTasksResponse, CancelTaskResponseTests.CancelTasksResponse> {

    @Override
    protected org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse createServerTestInstance(XContentType xContentType) {
        List<org.elasticsearch.tasks.TaskInfo> tasks = new ArrayList<>();
        List<TaskOperationFailure> taskFailures = new ArrayList<>();
        List<ElasticsearchException> nodeFailures = new ArrayList<>();

        for (int i = 0; i < randomIntBetween(1, 4); i++) {
            taskFailures.add(new TaskOperationFailure(randomAlphaOfLength(4), randomLongBetween(1, 3),
                new RuntimeException(randomAlphaOfLength(4))));
        }
        for (int i = 0; i < randomIntBetween(1, 4); i++) {
            nodeFailures.add(new ElasticsearchException(new RuntimeException(randomAlphaOfLength(10))));
        }

        for (int i = 0; i < randomIntBetween(1, 4); i++) {
            tasks.add(new org.elasticsearch.tasks.TaskInfo(
                new TaskId(randomAlphaOfLength(3), randomLong()),
                randomAlphaOfLength(4),
                randomAlphaOfLength(4),
                randomAlphaOfLength(10),
                new FakeTaskStatus(randomAlphaOfLength(4), randomInt()),
                randomLongBetween(1, 3),
                randomIntBetween(5, 10),
                false,
                new TaskId(randomAlphaOfLength(3), randomLong()),
                Map.of("x-header-of", "some-value")));
        }

        return new org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse(tasks, taskFailures, nodeFailures);
    }

    @Override
    protected CancelTaskResponseTests.CancelTasksResponse doParseToClientInstance(XContentParser parser) throws IOException {
        return CancelTasksResponse.fromXContent(parser);
    }

    @Override
    protected void assertInstances(org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksResponse serverTestInstance,
                                   CancelTaskResponseTests.CancelTasksResponse clientInstance) {
        // do a lot of validation between old and new fields 
    }

    public static class FakeTaskStatus implements Task.Status {

        final String status;
        final int code;

        public FakeTaskStatus(String status, int code) {
            this.status = status;
            this.code = code;
        }

        @Override
        public String getWriteableName() {
            return "faker";
        }

        @Override
        public void writeTo(StreamOutput out) throws IOException {
            out.writeString(status);
            out.writeInt(code);
        }

        @Override
        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
            builder.startObject();
            {
                builder.startObject("status_header");
                {
                    builder.field("status", status);
                }
                builder.endObject();
                builder.startObject("code_header");
                {
                    builder.field("code", status);
                }
                builder.endObject();
            }
            return builder.endObject();
        }
    }

    public static class TaskInfo {
        private static ConstructingObjectParser<TaskInfo, Void> PARSER  = new ConstructingObjectParser<>("task_info_cancel_tasks_response", true,
            constructingObjects -> {
                return new TaskInfo(
                    (String) (constructingObjects[0] + ":" + constructingObjects[1]),
                    (String) constructingObjects[2],
                    (String) constructingObjects[3],
                    (String) constructingObjects[4],
                    (long) constructingObjects[5],
                    (long) 0, // this is the running_time that is not coming back in this test but needs verification
                    (Map<String, Object>) constructingObjects[6],
                    (boolean) constructingObjects[7],
                    (String) constructingObjects[8],
                    (Map<String, String>) constructingObjects[9]);

            });

        static {
            PARSER.declareString(constructorArg(), new ParseField("node"));
            PARSER.declareLong(constructorArg(), new ParseField("id"));
            PARSER.declareString(constructorArg(), new ParseField("type"));
            PARSER.declareString(constructorArg(), new ParseField("action"));
            PARSER.declareString(constructorArg(), new ParseField("description"));
            PARSER.declareLong(constructorArg(), new ParseField("start_time_in_millis"));
            // not sure if we will get back running_time, cuz i dont know if human readable is set to true or not
            // when a es server is running, this will have to be verified
            // PARSER.declareLong(constructorArg(), new ParseField("running_time"));
            PARSER.declareObject(constructorArg(), (p, c)-> p.map(), new ParseField("status"));
            PARSER.declareBoolean(constructorArg(), new ParseField("cancellable"));
            PARSER.declareString(constructorArg(), new ParseField("parent_task_id"));
            PARSER.declareObject(constructorArg(), (p, c)-> p.map(), new ParseField("headers"));

        }

        private final String taskId;

        private final String type;

        private final String action;

        private final String description;

        private final long startTime;

        private final long runningTimeNanos;

        private final Map<String, Object> status;

        private final boolean cancellable;

        private final String parentTaskId;

        private final Map<String, String> headers;

        public TaskInfo(String taskId, String type, String action, String description, long startTime, long runningTimeNanos,
                        Map<String, Object> status, boolean cancellable, String parentTaskId, Map<String, String> headers) {
            this.taskId = taskId;
            this.type = type;
            this.action = action;
            this.description = description;
            this.startTime = startTime;
            this.runningTimeNanos = runningTimeNanos;
            this.status = status;
            this.cancellable = cancellable;
            this.parentTaskId = parentTaskId;
            this.headers = headers;
        }
    }

    public static class CancelTasksResponse {
        private static ConstructingObjectParser<CancelTasksResponse, Void> PARSER  = new ConstructingObjectParser<>("cancel_tasks_response", true,
            constructingObjects -> {
                List<Map<String, Object>> tasksFailures = (List<Map<String, Object>>) constructingObjects[0];
                List<Map<String, Object>> nodeFailures = (List<Map<String, Object>>) constructingObjects[1];
                List<TaskInfo> tasks = (List<TaskInfo>) constructingObjects[2];

                return new CancelTasksResponse(tasksFailures, nodeFailures, tasks);
            });

        static {
            PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> p.map(), new ParseField("task_failures"));
            PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> p.map(), new ParseField("node_failures"));
            PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> TaskInfo.PARSER.apply(p, c), new ParseField("tasks"));
        }

        private final List<TaskInfo> tasks;
        private List<Map<String, Object>> nodeFailures;
        private List<Map<String, Object>> taskFailures;

        public CancelTasksResponse(List<Map<String, Object>> taskFailures, List<Map<String, Object>> nodeFailures, List<TaskInfo> tasks) {
            this.taskFailures = taskFailures;
            this.nodeFailures = nodeFailures;
            this.tasks = tasks;
        }

        public static CancelTasksResponse fromXContent(XContentParser parser) throws IOException {
            return PARSER.parse(parser, null);
        }
    }
}

jesinity · 2019-10-31T16:59:32Z

I am still working on it, I think by monday I should reissue a merge request

hub-cap · 2019-11-01T13:12:28Z

Awesome news. I will be out for a week starting monday but ill do my best to review it when i see it.

jesinity · 2019-11-01T22:34:24Z

Hello, I issued a second commit.
Early warning, I partially applied your suggestions.
Overall I think it is far more polished!
Some comments.

I used the test you have provided as a guideline. Actually at the beginning, I tried to build an object "toXContent" to parse it back: that process was very error-prone, and was not showing the real Json structure of the response. So, in the end, I decided to do a parse from real JSON.
The test class is CancelTasksResponseTests, where you see all parsing end to end.
I did the status parsing, thanks for your suggestion! Works like a charm.
I still kept the TaskId (see comments above), but I simplified it.
to ease the pain to construct the CancelTaskRequest I introduced a builder. This way it is easier to manage this request creation.
Instead of using a empty singleton task id, and other empty fields here and there, I used Optionals .
I think it manages the parameters optionality far better.
I removed the NodesInfoData, that actually was causing parsing errors and was overall useless!
I simplified the ElasticSearchException transforming it in a simple container of the real exception info. Also, I removed the metadata that looked a bit overkill. I kept the hierarchy anyhow. (you see in the test 2 nested exceptions).
I put package access to some of the classes in the tasks package, so that they are used for internal reason and not exposed to the user. An Example is NodeData, that is there only for parsing purposes, but does not leak in the CancelTasksResponse.

hub-cap

This is great work. I think we have a few things to do to update it, and im weary of merging things that have large swaths of code w/o tests. Things like the ElasticsearchException, which have tons of logic in them, as well as the helpers that build some of the response code do not have tests. While they are potentially exercised in a response test, some of the things like the ElasticsearchException have so much nested logic that it might be worth testing it on its own. It might be worth you going thru the response classes and asking yourself if these classes are sufficiently exercised in the response test or not.

hub-cap · 2019-11-11T14:30:00Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksRequest.java

+            '}';
+    }
+
+    public static class Builder {


Lets remove this builder since some of the setters on the class itself already return this, and then add return this; to the methods in the Request that do not have them already.

maybe lets just remove the "return this" from the methods in the class and keep the builder, i think that makes more sense

hub-cap · 2019-11-11T14:33:06Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksResponse.java

+        this.taskGroups.addAll(buildTaskGroups());
+    }
+
+    private List<TaskGroup> buildTaskGroups() {


You are correct, I missed that this is all in the parent task to the CancelTasksResponse 👍

hub-cap · 2019-11-11T14:33:54Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/CancelTasksResponse.java

+        this.taskGroups.addAll(buildTaskGroups());
+    }
+
+    private List<TaskGroup> buildTaskGroups() {


I think its worth moving them to a "list task response" common class tho like in server side, so we dont have to move them (or duplicate them cuz maybe the author did not see them here).

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/NodeData.java

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/TaskGroup.java

...h-level/src/test/java/org/elasticsearch/client/documentation/TasksClientDocumentationIT.java

hub-cap · 2019-11-11T14:46:45Z

...h-level/src/test/java/org/elasticsearch/client/documentation/TasksClientDocumentationIT.java

-            request.setTaskId(new TaskId("nodeId1", 42)); //<1>
-            request.setActions("cluster:*"); // <2>
-            request.setNodes("nodeId1", "nodeId2"); // <3>
+            // request.setTaskId(new org.elasticsearch.client.tasks.TaskId("nodeId1", 42)); //<1>


any reason this is commented out?

no, I forgot to remove the line as I managed the emptiness through optionals, there is not the concept of "empty task id"

actually the ListTasksResponse contain all data that is CancelTasksResponse. Shall I add this new class in the hierarchy?

...h-level/src/test/java/org/elasticsearch/client/documentation/TasksClientDocumentationIT.java

hub-cap · 2019-11-11T14:50:41Z

...t/rest-high-level/src/test/java/org/elasticsearch/client/tasks/CancelTasksResponseTests.java

+import java.util.List;
+import java.util.Map;
+
+public class CancelTasksResponseTests extends ESTestCase {


The signature of my test was the following,

public class CancelTasksResponseTests extends AbstractResponseTestCase<CancelTasksResponse, CancelTasksResponseTests.CancelTasksResponse> {

and this test needs to also have that signature. Also, please do not just craft one message, you should be using "random" generators like in the other tests. It may require some additional methods to make the inner objects random, and you wont be able to use a nice .equals() method to test equality, but the point of these is to test that a server side response maps properly to a rest response.

a clarification here:

AbstractResponseTestCase<CancelTasksResponse, CancelTasksResponseTests.CancelTasksResponse>

the first CancelTasksResponse is the server side object (that also extends toXContent.
Shall I use this to create an instance that gets marshalled in XCOntent and parsed back?
I assumed you wanted to remove dependencies from the server, but maybe for testing purposes it is useful and more realistic to test the marshalling and unmarshalling of data.

If so, instead of using the CancelTasksResponseTests.CancelTasksResponse shall I use the real tasks counterpart, that implements all the fromXContent already (org.elasticsearch.client.tasks.CancelTasksResponse) ?
Otherwise there are duplications of the logic.

also, shall I remove my test or can I Keep it renaming it?

The test is not needed as is, but once it extends the AbstractResponseTestCase it will be good.

I assumed you wanted to remove dependencies from the server, but maybe for testing purposes it is useful and more realistic to test the marshalling and unmarshalling of data.

This is correct, we want to test the actual server object transforming to a client. There are a lot of tests that extend this so you can see the pattern for usage here.

If so, instead of using the CancelTasksResponseTests.CancelTasksResponse shall I use the real tasks counterpart,

Yes, the CancelTaskResponseTests subclass was only to show you how to use it, so it can be viewed as an example, and not code meant to be in the pull request.

ok I followed your boilerplate: actually I needed to create an adapter around the server side CancelTaskResponse because the default toXContent marshalls tasks in a different way, not under nodes but under tasks

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/TaskId.java

jesinity · 2019-11-13T12:44:00Z

I added two more commits to address the changes requested.

hub-cap

Minor nits, Im going to enable tests on this now so any updates will rerun the tests. Great work. Almost to the finish line!!!!

hub-cap · 2019-11-12T20:21:17Z

client/rest-high-level/src/main/java/org/elasticsearch/client/tasks/TaskInfo.java

+    public Map<String, Object> getStatus() {
+        return status;
+    }
+
    void noOpParse(Object s) {}


what is this used for?

It is is used down in the parser.
node:id pair is already being parsed in the NamedObjectParser identifier, but they are also repeated in the body.
So I parse them in the ObjectParser but ignore the result:

// already provided in constructor: triggering a no-op parser.declareString(TaskInfo::noOpParse, new ParseField("node")); // already provided in constructor: triggering a no-op parser.declareLong(TaskInfo::noOpParse, new ParseField("id"));

hub-cap · 2019-11-12T20:22:59Z

...h-level/src/test/java/org/elasticsearch/client/documentation/TasksClientDocumentationIT.java

@@ -162,13 +162,11 @@ public void testCancelTasks() throws IOException {
            // end::cancel-tasks-request

            // tag::cancel-tasks-request-filter
-            // request.setTaskId(new org.elasticsearch.client.tasks.TaskId("nodeId1", 42)); //<1>


having nothing between these tags will break the docs build.

ok, so is it safe to remove also the comments? the filtering capabilities is managed in the upper call

what I could to if to add an example to filter by TaskId, such as

CancelTasksRequest byTaskIdRequest = new org.elasticsearch.client.tasks.CancelTasksRequest.Builder() .withTaskId(new org.elasticsearch.client.tasks.TaskId("myNode",44L)) .build();

hub-cap · 2019-11-14T17:18:32Z

...high-level/src/test/java/org/elasticsearch/client/tasks/CancelTasksResponseParsingTests.java

+import java.util.List;
+import java.util.Map;
+
+public class CancelTasksResponseParsingTests extends ESTestCase {


I dont think we need this, given that we have a test that tests parsing of the response

ok, I can remove it

hub-cap · 2019-11-14T17:19:04Z

...est-high-level/src/test/java/org/elasticsearch/client/tasks/ElasticSearchExceptionTests.java

+import java.io.IOException;
+import java.util.List;
+
+public class ElasticSearchExceptionTests extends AbstractResponseTestCase<org.elasticsearch.ElasticsearchException,


Elasticsearch not ElasticSearch

ok! sometimes I camel more than I should

hub-cap · 2019-11-14T17:20:02Z

@elasticmachine ok to test

jesinity · 2019-11-14T18:46:24Z

did changes, I fixed the doc generation. I don't know why there are failing tasks. DO I need to rebase my changes to master?

jesinity · 2019-11-18T16:12:11Z

rebased but still I get tasks errors: this time only one fails! The error logs does not help me much

hub-cap · 2019-11-18T17:32:15Z

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

jesinity · 2019-11-19T17:44:28Z

good the checks have passed!
Are there more actions that I need to do on my side ?

hub-cap · 2019-11-21T19:24:04Z

Hey there is nothing else you need to do. Ill be doing the merging and backporting in the next day. Great work!

hub-cap · 2019-11-21T19:24:34Z

@elasticmachine update branch

jesinity · 2019-11-21T19:32:34Z

Neither do I have to rebase and squash the commits?
Amazing!
Thanks fo your review!

hub-cap · 2019-11-21T20:01:02Z

No need for any of that. Ive merged new commits so the tests rerun and ill be handling the squash thru the github UI.

hub-cap · 2019-11-22T20:38:14Z

Ive merged this, thank you for your work. Im backporting it now!

Adds support for proper cancel tasks parsing. Closes #45414

alpar-t added the :Core/Features/Java High Level REST Client label Sep 24, 2019

hub-cap self-assigned this Sep 25, 2019

hub-cap suggested changes Oct 11, 2019

View reviewed changes

hub-cap suggested changes Nov 11, 2019

View reviewed changes

jesinity mentioned this pull request Nov 12, 2019

v #49010

Closed

hub-cap suggested changes Nov 14, 2019

View reviewed changes

jesinity and others added 4 commits November 18, 2019 17:04

Fixing HLRC parsing of CancelTasks response (elastic#45414)

59f53dd

doing the fixes

5680adf

fixing tests.... in progress

dbcf601

fixing tests done

7314ddd

jesinity added 2 commits November 18, 2019 17:04

fixing comments for tests and doc

e1e3e84

fixing doc generation

c3ae790

jesinity force-pushed the cancel-tasks-fix branch from 7a7aeab to c3ae790 Compare November 18, 2019 16:04

hub-cap approved these changes Nov 21, 2019

View reviewed changes

Merge branch 'master' into cancel-tasks-fix

af28266

hub-cap changed the title ~~Fixing HLRC parsing of CancelTasks response (#45414)~~ Fix HLRC parsing of CancelTasks response Nov 22, 2019

hub-cap merged commit 52003a2 into elastic:master Nov 22, 2019

hub-cap added >bug v7.6.0 v8.0.0 backport pending labels Nov 22, 2019

hub-cap pushed a commit that referenced this pull request Nov 22, 2019

Fix HLRC parsing of CancelTasks response (#47017)

c9eba17

Adds support for proper cancel tasks parsing. Closes #45414

hub-cap removed the backport pending label Nov 22, 2019

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Fix HLRC parsing of CancelTasks response #47017

Fix HLRC parsing of CancelTasks response #47017

Conversation

jesinity commented Sep 24, 2019 • edited Loading

ghost commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

hub-cap commented Sep 25, 2019

hub-cap commented Oct 9, 2019

jesinity commented Oct 10, 2019

hub-cap commented Oct 10, 2019 • edited Loading

There is a newer copy of this code below

jesinity commented Oct 11, 2019 • edited Loading

hub-cap commented Oct 11, 2019

hub-cap commented Oct 11, 2019 • edited Loading

hub-cap commented Oct 11, 2019

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity commented Oct 20, 2019

hub-cap commented Oct 21, 2019

hub-cap commented Oct 21, 2019

hub-cap commented Oct 21, 2019 • edited Loading

jesinity commented Oct 31, 2019

hub-cap commented Nov 1, 2019

jesinity commented Nov 1, 2019 • edited Loading

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity commented Nov 13, 2019

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesinity Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Nov 14, 2019

jesinity commented Nov 14, 2019

jesinity commented Nov 18, 2019 • edited Loading

hub-cap commented Nov 18, 2019

jesinity commented Nov 19, 2019

hub-cap commented Nov 21, 2019

hub-cap commented Nov 21, 2019

jesinity commented Sep 24, 2019 •

edited

Loading

hub-cap commented Oct 10, 2019 •

edited

Loading

jesinity commented Oct 11, 2019 •

edited

Loading

hub-cap commented Oct 11, 2019 •

edited

Loading

jesinity Oct 20, 2019 •

edited

Loading

jesinity Oct 20, 2019 •

edited

Loading

hub-cap commented Oct 21, 2019 •

edited

Loading

jesinity commented Nov 1, 2019 •

edited

Loading

jesinity Nov 12, 2019 •

edited

Loading

jesinity Nov 14, 2019 •

edited

Loading

jesinity commented Nov 18, 2019 •

edited

Loading