Remove tricky switch in bulk #80624
Remove tricky switch in bulk #80624
Conversation
Now that we have elastic#79394 and elastic#79472 the switch statement in `TransportBulkAction` is mostly just assigning routing and doing some stuff with INDEX and UPDATE requests. This pulls the routing stuff into a single call and calls out the index-only stuff. Now, at least, it's clearer that only INDEX and UPDATE are special.
nik9000
added
>non-issue
:Distributed Indexing/CRUD
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
@elasticmachine update branch |
|
Sorry for breaking the BWC tests, it's fixed now in master in 77a8ab5 |
| prohibitAppendWritesInBackingIndices(docWriteRequest, metadata); | ||
| prohibitCustomRoutingOnDataStream(docWriteRequest, metadata); | ||
| IndexRequest indexRequest = (IndexRequest) docWriteRequest; | ||
| indexRequest.resolveRouting(metadata); |
There was a problem hiding this comment.
I think the resolveRouting(...) method can now also be removed from IndexRequest class? (looks this was the only usage).
There was a problem hiding this comment.
Doh! I had done that when I was working on it and had lost it in the shuffle. I'll add it back
| throw new AssertionError("request type not supported: [" + docWriteRequest.opType() + "]"); | ||
| if (docWriteRequest.opType() == OpType.CREATE || docWriteRequest.opType() == OpType.INDEX) { | ||
| prohibitAppendWritesInBackingIndices(docWriteRequest, metadata); | ||
| prohibitCustomRoutingOnDataStream(docWriteRequest, metadata); |
There was a problem hiding this comment.
@martijnvg I wonder if it is not a bug that we do not check this for all request types, including update/delete? Maybe better handled in a follow-up, but would be nice to move out of this "if". And perhaps then let prohibitAppendWritesInBackingIndices handle the ops itself to simplify this code (the filter for CREATE/INDEX here is really only partial anyway, so might as well do that inside the method).
There was a problem hiding this comment.
Yes, I think prohibitCustomRoutingOnDataStream() should be checked for all types of request, irregardless whether these target the data stream of one of the backing indices. I think only prohibitAppendWritesInBackingIndices() should be checked in this if statement. I can work on that in a followup.
There was a problem hiding this comment.
Neat! I was hoping writing it like this would make stuff like this more obvious. I'll leave this to @martijnvg's follow up though.
| } | ||
| docWriteRequest.routing(metadata.resolveWriteIndexRouting(docWriteRequest.routing(), docWriteRequest.index())); | ||
| if (docWriteRequest.opType() == OpType.CREATE || docWriteRequest.opType() == OpType.INDEX) { | ||
| ((IndexRequest) docWriteRequest).process(); |
There was a problem hiding this comment.
Why not add DocWriteRequest.process() too to avoid the if here?
There was a problem hiding this comment.
I was looking at that. Flipping it over and over like it was a serving bowl I didn't need at a yard sale. I'll put it back that way and see if you like.
There was a problem hiding this comment.
Ah ha! The thing that was bothering me last time was that I tried to merge the alias routing setting into DocWriteRequest.process but that wasn't working right because we need to call DocWriteRequest.process on the data nodes after update requests have been translated. I didn't think to myself "what if I didn't try to merge those things?"
👍 |
nik9000
force-pushed
the
transport_bulk_no_switch
branch
from
a881f84 to
43f7496
Compare
* Always check whether it is prohibited to use custom routing on a data stream. * Always invoke prohibitAppendWritesInBackingIndices(...), but in the method check whether the operation is of type index or create. Follow-up from elastic#80624.
* Always check whether it is prohibited to use custom routing on a data stream. * Always invoke prohibitAppendWritesInBackingIndices(...), but in the method check whether the operation is of type index or create. Follow-up from #80624.
Now that we have #79394 and #79472 the switch statement in
TransportBulkActionis mostly just assigning routing and doing somestuff with INDEX and CREATE requests. This pulls the routing stuff into
a single call and calls out the index-only stuff. Now, at least, it's
clearer that only INDEX and CREATE are special.