Add a BWC layer to BulkItemRequest so 6.0 won't need to mutate it. #25512
Conversation
| request = DocWriteRequest.readDocumentRequest(in); | ||
| if (in.readBoolean()) { | ||
| primaryResponse = BulkItemResponse.readBulkItem(in); | ||
| if (in.getVersion().onOrAfter(Version.V_5_6_0_UNRELEASED)) { |
There was a problem hiding this comment.
I am not sure we should mix onOrAfter and before in the same file, it's confusing?
There was a problem hiding this comment.
Agreed - I wanted to add a 6.0.0 here and keep the wire level logic symmetrical. The 6.0.0 constant is not available on 5.x, which I wanted to discuss / understand why.
There was a problem hiding this comment.
We discussed this via another channel, @bleskes will work on removing this guard and obviating the need for even considering adding a 6.0.0 version constant.
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
Left a comment about assert placement
| DocWriteRequest.writeDocumentRequest(out, request); | ||
| out.writeOptionalStreamable(primaryResponse); | ||
| out.writeBoolean(ignoreOnReplica); | ||
| assert ignoreOnReplica == |
There was a problem hiding this comment.
It feels weird to place this assert here, rather than where ignoreOnReplica is actually set, it also would make debugging more difficult, because instead of a stacktrace letting you know where it was incorrectly set, it only manifests once serialized.
How would you feel about combining setPrimaryResponse and setIgnoreOnReplica so they are entangled and an exception is thrown if an invalid state is configured when the primary response is set? Perhaps something like:
void setPrimaryResponse(BulkItemResponse primaryResponse, boolean ignoreOnReplica) {
assert ignoreOnReplica ==
(primaryResponse != null &&
(primaryResponse.isFailed() || primaryResponse.getResponse().getResult() == DocWriteResponse.Result.NOOP)
) :
"unexpected ignoreOnReplica value. primaryResponse [" + primaryResponse + "], primaryResponse ["
+ (primaryResponse == null ? "null" : XContentHelper.toString(primaryResponse)) + "]";
this.primaryResponse = primaryResponse;
this.ignoreOnReplica = ignoreOnReplica;
}It could also be configured as a regular exception there also, instead of an assert (IllegalArgumentException). It would also have the benefit of not leaving the object in an illegal state when an exception was thrown (nice to be clean)
|
@dakrone thanks for your suggestion. Instead of changing the API, I went ahead and removed the ignore on replicas field completely. can you take another look? |
| primaryResponse = BulkItemResponse.readBulkItem(in); | ||
| // This is a bwc layer for 6.0 which no longer mutates the requests with these | ||
| // Since 5.x still requires it we do it here. Note that these are harmless | ||
| // as both operations are idempotent. This is something we rely for and assert on |
|
Thx @jasontedor @dakrone |
4 participants
This is a companion PR to #25511 . See there for more explanation and background.