Remove BulkIndexByScrollResponseContentListener

[Mpdreamz]

Its causes the reindex and update_by_query to be the only API that returns a body on errors thats not a structured error.

It also tries to do the right thing by setting the status code to the highest statuscode of all the individual failures which I don't think is terribly useful to consumers either.

It be nicer if these API's behaved like the _bulk and return a 200 if the request was received properly and each response has to handle individual failures (whether conflicts=proceed or not) with an errors boolean.

[nik9000]

Its causes the reindex and update_by_query to be the only API that returns a body on errors thats not a structured error.

This I'm happy to fix but I don't really understand.

It also tries to do the right thing by setting the status code to the highest statuscode of all the individual failures which I don't think is terribly useful to consumers either.

This I really really really want to keep. Not only does it handle important cases like "your query failed for some reason" but I really like returning a REST status code when the request terminated early that explains why. Returning 200 when we bailed early isn't cool.

[Mpdreamz]

Most responses on a bad status code (400, 409, 500 etcetera) will return the throwable as structured error instead of the same response as on a 200.

Returning 200 when we bailed early isn't cool.

But is exactly what we do for _bulk, _mget _msearch and the likes. They return a 200 with a valid json response strucure with"errors": true. Not doing this for reindex and update by query makes them the odd ones out and also the only ones that'll return a valid json response structure when the http status code is not 200.

I personally agree that a status code indicating a (partial) failure for all of these API's would be an improvement but we'd need to settle on a single status code (e.g 400) rather then stealing a status code from one of the individual sub request failures.

[nik9000]

I don't think _reindex and _update_by_query can just be like _bulk, _mget, and _msearch. The latter APIs still attempt all the things you asked them to do even if there was a failure. The former don't. Personally I'm not happy that we return 200 when those APIs only partially succeed but that is a battle for another time.

Is the trouble the reindex doesn't return a single error field like the other APIs? If it did that would it make everything work better? We could cook up something like "aborted due to other errors error" if it'd make things fit.

I think it is a good thing reindex returns its progress when it aborts due to an error because that data is useful. Well, not all of it is, but stuff like the total hits and how many of each operation in performed is.

[Mpdreamz]

I don't think _reindex and _update_by_query can just be like _bulk, _mget, and _msearch. The latter APIs still attempt all the things you asked them to do even if there was a failure.

That does not mean _reindex and update_by_query stops at the first error, a batch can have multiple failures and it that sense is exactly like the _bulk etcerera in my opinion.

And because it can have multiple failures picking the highest is just as meaningless as 200.

Is the trouble the reindex doesn't return a single error field like the other APIs? If it did that would it make everything work better?

I rather we make it behave the same as mget and family, either by return a 200 with "errors" : true or a common single http status code for all of these API's with (partial) failures

I think it is a good thing reindex returns its progress when it aborts due to an error because that data is useful. Well, not all of it is

I agree!, the exception that is rendered on "error" can include more data by overriding innerToXContent like the SearchPhaseExecutionException does

From a pure RESTful perspective i'd agree the current API is better, right now though these are the only API's I have to special case. In our client we have a dedicated error reader for bad responses that'll shortcircuit deserialization alltogether. Whilst not something we can not work around I rather treat all elasticsearch responses the same.

[clintongormley]

Discussed in Fix it Friday. A bulk or mget operation is a wrapper of individual items, so it makes sense for a bulk operation with errors to return 200 (with errors listed in the body) because the coordinating node has done its job correctly.

Reindex on the other hand aborts when it encounters an error, and should return an error code. I agree with @Mpdreamz that this should be a 400 or a 500 (depending on the error that was thrown) but it shouldn't just choose the highest error code. These error codes have meaning for the clients (eg should i retry this request) and just because one error code happens to be 429 or 503 doesn't mean that the client should retry the reindexing request.

Regarding the structured body... I think the information that is passed back is useful, but it is inconsistent with some responses (eg bulk) but inconsistent with others. I considered wrapping the info inside a structured error body, but that feels forced.

My Perl client looks for a structured error but, failing that, falls back to including the whole body in the exception. I'm not sure what other clients do here.

[nik9000]

So what is the change you want? Change the response code calculations to be "return the code if all errors have the same code, if two errors have different codes and any are 500-level then return 500. Otherwise return 400?"

[clintongormley]

No, return 500 if any errors are 50*, or 400 otherwise. (open to discussion here).

[nik9000]

Ok, so:
0. If there are no errors return 200.

1. If there are any 500 level statuses then return 500.
2. Return 400.

[clintongormley]

i think this makes sense. @Mpdreamz ?

[nik9000]

@Mpdreamz, do you like my proposal in #17548 (comment) ?