response.getTasks() incorrectly parses _tasks/_cancel response

[n0othing]

Elasticsearch version (bin/elasticsearch --version): 6.8.1

Plugins installed: []

JVM version (java -version):

java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)

OS version (uname -a if on a Unix-like system):

Darwin Kernel Version 18.7.0: Thu Jun 20 18:42:21 PDT 2019; root:xnu-4903.270.47~4/RELEASE_X86_64 x86_6

Description of the problem including expected versus actual behavior:

Our example docs [1] show how to use the response from a _tasks/_cancel API call via the HLRC. However, the parser [2] is looking for a top level tasks object, but it's really nested under nodes.* in the actual response:

{
  "nodes" : {
    "xRYjtvivTlKXuXDUL0Hhcw" : {
      "name" : "instance-0000000004",
      "transport_address" : "172.25.162.9:19987",
      "host" : "172.25.162.9",
      "ip" : "172.25.162.9:19987",
      "roles" : [
        "master",
        "data",
        "ingest"
      ],
      "attributes" : {
        "logical_availability_zone" : "zone-0",
        "server_name" : "instance-0000000004.2ab4c8015d104467862e200444aeb2ac",
        "availability_zone" : "us-east-1e",
        "xpack.installed" : "true",
        "region" : "us-east-1",
        "instance_configuration" : "aws.data.highio.i3"
      },
      "tasks" : {
        "xRYjtvivTlKXuXDUL0Hhcw:1481971" : {
          "node" : "xRYjtvivTlKXuXDUL0Hhcw",
          "id" : 1481971,
          "type" : "transport",
          "action" : "indices:data/write/reindex",
          "start_time_in_millis" : 1565108484002,
          "running_time_in_nanos" : 18328639067,
          "cancellable" : true,
          "headers" : { }
        }
      }
    }
  }
}

[1] https://www.elastic.co/guide/en/elasticsearch/client/java-rest/6.8/java-rest-high-cluster-cancel-tasks.html#_cancel_tasks_response
[2] https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java#L96-L99

[elasticmachine]

Pinging @elastic/es-core-features

[hub-cap]

Thank you for the submission.

Whomever is going to be taking on this work, it will not be a small task. I would like to see client classes created for the request and response, to decouple from the server side classes, as the first step. These classes will (possibly) be leaner than the server side classes as well. The validation should be moved into constructors where possible as well. Then the API will need to be deprecated using the old classes and new methods for the new classes. <3 to whomever would like to take it on 🗡️ 💥

[jesinity]

I never contributed actively to the project bit I think I could Pick It up.

About the approach suggested, you mean that Request and Response issued by the HLRC should not be those used in the server code base?
If so I think this approach Is correct.

Looking to other issues in HLRC such as #41228 and #29184 the approach you suggest seems the only way to decouple the HLRC from server dependencies, as the server requests and responses are tighty bound to core libraries and classes ( such ascfor example ElasticSearchException, ExceptionHelper, StreamInput and StreamOutput ).

[hub-cap]

@jesinity It would be great for you to do. You are correct in that many of those classes do not need to be present in the client. We also sometimes relax the parameters, for instance an enum server side might just be a string client side without strict validation (it helps for backward comparability). Also, if you look at some of the tests, you can see that unless a request has a body, it should not have a toXContent, because its not needed. The tests implement that portion of the code.

[hub-cap]

A good example of not needing complex objects is the TaskInfo. For instance you could just have a String for the taskId, instead of a complex object. Im sure there are others, just be weary of anything thats more complex than a primitive, unless its storing an internal object with multiple inner fields

[hub-cap]

@jesinity do you think you will be tackling this soon? Or are you in progress? It seems like there might be a few bugs in this API and I would like to see about getting it sorted soon, if you cannot lend time to it.

[jesinity]

yes, I am starting tackling it, I hope I will be able to send a WIP soon.

[hub-cap]

excellent!!

[jesinity]

Hi @hub-cap , here is a work in progress of this fix:
https://github.com/elastic/elasticsearch/compare/master...jesinity:cancel-tasks-fix?expand=1

Some comments:

1. 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.
2. I did not manage to create complex statuses with failures or exceptions, but I went through how the response is generated server side.
3. I did not touch yet the ListTasksResponse to the other calls of the tasks api are still using old classes.
4. I did some unit tests I did not commit yet: some parsing tests are enough for the testing purposes?
5. 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.

[jesinity]

I went a bit ahead and opened a pull request

[jesinity]

in case when this it is done I think I can tackle the #46062

[hub-cap]

Thanks and I will be getting to this soon. its been a packed few weeks, but im finally out of the woods and can focus on some OSS stuff and review your stuff @jesinity