Introduce a common base response class to all single doc write ops #15334
Conversation
|
@ywelsch can you please take a look? |
| /** | ||
| * A base class for the response of a write operation that involves a single doc | ||
| */ | ||
| public abstract class DocWriteResponse extends ReplicationResponse implements ToXContent { |
There was a problem hiding this comment.
call this DocOperationResponse (as it includes deletes)?
There was a problem hiding this comment.
I think deleted is a write operation? DocOperationResponse as a name does cover Get, which isn't part of this game...
|
Minor comments, o.w. LGTM |
IndexResponse, DeleteResponse and UpdateResponse share some logic. This can be unified to a single DocWriteResponse base class. On top, some replication actions are now not about write operations anymore. This commit renames ActionWriteResponse to ReplicationResponse Last some toXContent is moved from the Rest layer to the actual response classes, for more code re-sharing. This is ported over from the `feature/seq_no` branch to make maintaining that branch simpler
bleskes
force-pushed
the
doc_write_response
branch
from
7afde9a to
952c38f
Compare
|
@ywelsch @martijnvg I addressed all your feedback and rebased on master (there was a big change to replication action there). Can you take another look? |
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field(Fields.FOUND, isFound()); | ||
| super.toXContent(builder, params); |
There was a problem hiding this comment.
switch these 2 lines so that id, version etc. appear first?
There was a problem hiding this comment.
This how it used to be (found first), and I tend to agree - the found flag is the mode important one. Might be inconsistent but it's not my goal to change this in this PR (requires research on the other endpoints as well)
There was a problem hiding this comment.
ok, just noticed this in comparison to IndexResponse.
|
More lines removed than added -> I like this. |
|
Good stuff. LGTM |
|
@ywelsch pushed another round with your suggestions. |
|
LGTM |
IndexResponse, DeleteResponse and UpdateResponse share some logic. This can be unified to a single DocWriteResponse base class. On top, some replication actions are now not about write operations anymore. This commit renames ActionWriteResponse to ReplicationResponse
Last some toXContent is moved from the Rest layer to the actual response classes, for more code re-sharing.
This is ported over from the
feature/seq_nobranch to make maintaining that branch simpler