IndexAction to return DocWriteResponse

[romseygeek]

IndexRequests can sometimes return an UpdateResponse rather than an IndexResponse if
there is an ingest pipeline that drops documents. This commit changes IndexAction to
return their common superclass DocWriteResponse.

[romseygeek]

cc @DaveCTurner - this seems to work, but it is pretty invasive, which makes me think maybe allowing IndexResponse to be a NO_OP might be simpler?

The REST status that should be returned when we just drop a document is something else I'm unsure of. At the moment we return 200 OK (rather than 201 CREATED) but that doesn't quite feel right to me. Maybe 202 ACCEPTED is better?

[DaveCTurner]

LGTM

Certainly a noisy change, but I wouldn't call it invasive - it's pretty superficial. I'm a little surprised that no callers cared what kind of response they're getting, but I don't see any here.

[DaveCTurner]

Just to be safe we can preserve today's behaviour in prod whilst still failing tests if anyone calls this in future:

Suggested change

	super(NAME, in -> { throw new UnsupportedOperationException(); });
	super(NAME, in -> {
	assert false : "might not be an IndexResponse!";
	return new IndexResponse(in);
	});

[DaveCTurner]

At the moment we return 200 OK (rather than 201 CREATED) but that doesn't quite feel right to me. Maybe 202 ACCEPTED is better?

I like this bikeshed the colour it is already :)

RFC 2616 section 10.2.3 says:

10.2.3 202 Accepted

   The request has been accepted for processing, but the processing has
   not been completed.  The request might or might not eventually be
   acted upon, as it might be disallowed when processing actually takes
   place. There is no facility for re-sending a status code from an
   asynchronous operation such as this.

   The 202 response is intentionally non-committal. Its purpose is to
   allow a server to accept a request for some other process (perhaps a
   batch-oriented process that is only run once per day) without
   requiring that the user agent's connection to the server persist
   until the process is completed. The entity returned with this
   response SHOULD include an indication of the request's current status
   and either a pointer to a status monitor or some estimate of when the
   user can expect the request to be fulfilled.

I don't think that fits our usage.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[romseygeek]

I'm a little surprised that no callers cared what kind of response they're getting, but I don't see any here.

IndexResponse adds no methods or fields to DocWriteResponse so I don't think it would actually be possible for it to make a difference! UpdateResponse has an optional get field so there is at least a difference there.

[romseygeek]

I've dug a little further in to what it is that's using the Writeable.Reader from ActionType, and it appears that the only real use is in the RemoteClusterAwareClient, which may use IndexRequest/Response as part of CCR? It isn't an area of the code I know well, so I wonder if we are in fact testing that CCR works with ingest pipelines that drop documents.

[DaveCTurner]

it appears that the only real use is in the RemoteClusterAwareClient,

Yes, I'm really keen to separate out the few remote-cluster actions that still need their requests & responses to be de/serializable so we can get rid of all this dead code (cc @ldematte). But no, CCR doesn't send single-doc indexing requests over the wire.

[romseygeek]

@elasticmachine update branch

[DaveCTurner]

I opened #100111 to capture the thing about this dead code everywhere, and #100112 to make it a little easier to mark (at least new) dead code as dead.

[DaveCTurner]

LGTM

[romseygeek]

@elasticmachine update branch