GetTaskResponse does not expose Failures for reindex task

[octocat-mona]

NEST/Elasticsearch.Net version:
7.0.1

Elasticsearch version:
7.2.0

Description of the problem including expected versus actual behavior:
Expected:

• IsValid to be false when "failures" : [] response is not empty (just like ReindexOnServerResponse.IsValid
• Be able to read the response.failures of a reindex task

Actual:

• IsValid is true when "failures" : [] is not empty

Steps to reproduce:

1. Index documents into source index
2. Create destination index with 0 replicas
3. Create reindex task with WaitForCompletion(false)
4. Poll status using GetTaskAsync using the TaskId and check IsValid and Completed
5. Disconnect data node containing the shards of the indices

Response from _tasks/<id> includes failures (removed some fields for readability):

{
  "completed" : true,
  "task" : {
    "action" : "indices:data/write/reindex",
    "status" : {
      "total" : 2695491,
      "updated" : 0,
      "created" : 0,
      "deleted" : 0,
      "batches" : 499,
      "version_conflicts" : 498000,
      "noops" : 0
    },
    "description" : "reindex from [my-index-1] to [my-index-2]",
    "cancellable" : true
  },
  "response" : {
    "took" : 656338,
    "timed_out" : false,
    "total" : 2695491,
    "updated" : 0,
    "created" : 0,
    "deleted" : 0,
    "batches" : 499,
    "version_conflicts" : 498000,
    "noops" : 0,
    "failures" : [
      {
        "index" : "my-index-2",
        "type" : "_doc",
        "id" : "bowFbmoBnnQlcVPVmn7H",
        "cause" : {
          "type" : "node_not_connected_exception",
          "reason" : "[instance-0000000070][1.2.3.4:19437] Node not connected"
        },
        "status" : 500
      }
    ]
  }
}

[russcam]

Thanks for opening @KPStolk.

• IsValid to be false when "failures" : [] response is not empty (just like ReindexOnServerResponse.IsValid

I'm not sure we'd want to map .IsValid to this. failures is on the response object, which will be the response for the action initiated by the task, so will vary depending on what the task does.

I think we should expose the "response" on the GetTaskResponse though, likely as an internal LazyDocument field, and allow the consumer to deserialize to the response of choice

[russcam]

There's some further complexity to consider with exposing "response" on GetTaskResponse. The ApiCallDetails will be null on the deserialized "response" object and there will be missing information such as

• no HTTP response status code
• no Url endpoint

etc. This is because this information is not exposed on the object itself but determined from the HTTP request/response, which in the case of a GetTaskResponse will relate to the HTTP request/response for the Get Task API. This would mean that .IsValid cannot not be relied upon in the deserialized "response" because there is missing information.

[russcam]

I've opened #3966

[octocat-mona]

@russcam Thanks for the fix.

IsValid usually does check when there is a collection which can contains errors or failures, for example:

• ReindexOnServerResponse.IsValid: is false when there are any Failures
• BulkResponse.IsValid: is false when there are any ItemsWithErrors

The difference here is indeed that a task can optionally have a response object with failures.

But if I'm at least able to easily check for any failures myself it won't be an issue for me, is it possible to deserialize LazyDocument as an ReindexOnServerResponse object?

[russcam]

IsValid usually does check when there is a collection which can contains errors or failures, for example:

ReindexOnServerResponse.IsValid: is false when there are any Failures
BulkResponse.IsValid: is false when there are any ItemsWithErrors

This is true, but .IsValid usually combines these checks with the base .IsValid implementation on ResponseBase, e.g. 7.x

elasticsearch-net/src/Nest/CommonAbstractions/Response/ResponseBase.cs

Lines 89 to 96 in 6c6d932

	public virtual bool IsValid
	{
	get
	{
	var statusCode = ApiCall?.HttpStatusCode;
	if (statusCode == 404) return false;
	return (ApiCall?.Success ?? false) && ServerError == null;
	}}

So, for the "response" on GetTaskResponse, which you'll be able to deserialize to a specific TResponse with #3966 using .GetResponse<TResponse>()

var response = client.Tasks.GetTask("some task id for a reindex on server API call");

var reindexResponse = response.GetResponse<ReindexOnServerResponse>();

So what I meant in the last comment is that .IsValid on reindexResponse, which has been deserialized from the "response" object, will not be valid, because there are no ApiCallDetails associated with it. I don't think this is a big issue, but it may be surprising behaviour to some. The ApiCallDetails associated with the response as a whole are related to the GetTaskResponse, so I think it would be incorrect to transpose these onto any response deserialized from "response" property.

[octocat-mona]

@russcam I think it's logical that .IsValid won't return true when deserialized using GetResponse<ReindexOnServerResponse>() but indeed it might be confusing, at least it's the easiest way to check for failures I think.

This is true, but .IsValid usually combines these checks with the base .IsValid implementation on ResponseBase, e.g. 7.x

I don't know what kind of tasks there are but if there are just a few like _reindex which can return a "response" you could override .IsValid in GetTaskResponse and check if there are any failures?
This way you can combine .IsValid and check for reindex failures in case it's a reindex task.

For example:

// GetTaskResponse class

public override bool IsValid
{
    get
    {
        if (base.IsValid)
        {
            return !(GetResponse<ReindexOnServerResponse>()?.Failures?.Any()).GetValueOrDefault();
        }
        return false;
    }
}

But if this is too tricky in your opinion then we should just check this besides .IsValid in calling code just like you showed.