Bulk operations reports "success" despite underlying errors in the response

[bitsofinfo]

Submitting various Bulk requests in Jest.

I get a client from the client factory and call execute() on it with my Bulk request, I get back a JestResult and check isSucceeded() on it which reports true, yet when I inspect the JestResult jsonString property I see all these errors in the result.

I don't think JestResult should treat this as a successful result. If some failed and some succeeded I think there should be a way to interrogate the JestResult (via another flag) like "partial success/error" etc to hint the caller to check the raw result string...

{"took":206,"errors":true,"items":[{"index":{"_index":"my-type-dn-v1-b1","_type":"user","_id":"71918fef-a24b-4d79-b93c-f78fcc13abf5","status":400,"error":"MapperParsingException[failed to parse [department]]; nested: ElasticsearchIllegalArgumentException[unknown property [departmentLabel]]; "}},{"index":{"_index":"my-type-dn-v1-b1","_type":"user","_id":"56553391-0810-47c7-a39c-bcbbde34e36b","status":400,"error":"MapperParsingException[failed to parse [department]]; nested: ElasticsearchIllegalArgumentException[unknown property [departmentLabel]]; "}},{"index":{"_index":"my-type-dn-v1-b1","_type":"user","_id":"c6c6142d-45b3-427e-9d22-53e26cd666cf","status":400,"error":"MapperParsingException[failed to parse [department]]; nested: ElasticsearchIllegalArgumentException[unknown property [departmentLabel]]; "}}]}

[kramer]

You are right on that Bulk does not handle the error case in the most ideal way now. For the solution though, I think fitting into the existing error behaviour would more consistent than introducing a new flag. I see that the response already has an errors field, we can set this as the isSucceeded value. Then JestResult.isSuceeded for bulk would return false even if one operation in bulk fails and other 4 succeeds; and we can put the individual error messages from operations into the errorMessage field. What do you think?

[bitsofinfo]

I think that would be fine, ideally it would be good to be able to easily get a List of failed doc _ids

[kramer]

Well you can have multiple operations on the same doc _id, so to get over that we would need to return individual operations in a structured way. My rough idea is:

• assume class FailedBulkOperation contains fields DocumentId and OperationType.
• BulkResponse.getFailedOperations returns collection of FailedBulkOperation instances.

edit: If I'm not mistaken, you can actually have same operation on same id multiple times and there is no information to identify them uniquely in the response... Just a strange case to keep in mind.

[ctrimble]

@kramer with the way this issue was resolved and the position being taken on issue #220, it is going to be hard to tell if a bulk operation itself failed and will need to be retried or if it is just a problem with a few documents.

[kramer]

@ctrimble Currently one would do:

if(action.isSucceeded)
    // all good
else if(action. getFailedItems > 0)
    // failed due to content 
else
    // failed HTTP

How would having access to http code change this check?

[ctrimble]

I am still having trouble with clear documentation on failures in ES, but digging through the different tickets that are around, it seems like 503 is used to signal that a client should retry a request.

The code you posted does seem workable. It did not seem intuitive that getFailedItems would return something meaningful if the bulk operation itself had an issue.

[kramer]

It did not seem intuitive that getFailedItems would return something meaningful if the bulk operation itself had an issue.

It would return something meaningful only if the request was properly formatted (then evaluated by server) but some of the operations in bulk failed. If the whole request was malformed then ES returns 4XX iirc, which would go into the very last else statement in previous pseudocode. Same goes for 5XX responses. And yes at that point you would not be able to distinguish between a 4XX and a 5XX other than the information in getErrorMessage.

Original concern (at least as I understood it) and my suggestions were only for distinguishing between 4XX response and (properly evaluated yet) failed operation response. I see now that is not all but there is also a need to distinguish between 4XX vs 5XX. Since the interest point in that distinction is being able to retry, I'd suggest utilising the retry strategies of the http client such as DefaultServiceUnavailableRetryStrategy (i.e.: adding and making them configurable for jest client).
What do you think?

[ctrimble]

I am not sure about the details of the retry strategy, but I think that abstracting the status code from users is not the best thing. Helper methods for testing if the code is 2XX, 3XX, etc would be good, but when things aren't working, it helps to have access to the actual status code.

[kramer]

Update: response code is now added to result class. After thinking it over I agree with @ctrimble that if it's gonna make user's life easier there's no reason to not have this change considering no effort is required.