Move routing calculation #79394
Merged
Move routing calculation #79394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nik9000
added
>non-issue
:Distributed Indexing/Distributed
elasticmachine
added
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
This slightly moves the routing calculation out of a difficult to reason about `switch` statement and into real OO method implementation. Its a tiny tiny change but it makes me feel much better about it.
nik9000
commented
Oct 18, 2021
| break; | ||
| case UPDATE: | ||
| TransportUpdateAction.resolveAndValidateRouting(metadata, concreteIndex.getName(), | ||
| (UpdateRequest) docWriteRequest); | ||
| shardId = indexRouting.updateShard(docWriteRequest.id(), docWriteRequest.routing()); | ||
| break; | ||
| case DELETE: | ||
| docWriteRequest.routing(metadata.resolveWriteIndexRouting(docWriteRequest.routing(), docWriteRequest.index())); | ||
| // check if routing is required, if so, throw error if routing wasn't specified | ||
| if (docWriteRequest.routing() == null && metadata.routingRequired(concreteIndex.getName())) { | ||
| throw new RoutingMissingException(concreteIndex.getName(), docWriteRequest.id()); | ||
| } |
There was a problem hiding this comment.
I played around a little bit and most of this switch statement can be removed I think. But I want to do it bit by bit because its in an important bit of code.
There was a problem hiding this comment.
I opened #79472 which slices out a lot of the rest of this switch statement. I believe all that is left is the validation around data streams.
|
run elasticsearch-ci/part-2 |
henningandersen
approved these changes
Oct 27, 2021
| @@ -720,6 +721,11 @@ public boolean isRequireAlias() { | |||
| return requireAlias; | |||
| } | |||
|
|
|||
| @Override | |||
| public int route(IndexRouting indexRouting) { | |||
There was a problem hiding this comment.
Maybe add comment here that resolveRouting and process must be called prior to this.
Also I think we can cement parts of that by asserting that id != null?
There was a problem hiding this comment.
👍
I'd sort of been hoping to fold those two methods together. But that is a thing for another time.
…uting_from_source_clean_1
…uting_from_source_clean_1
nik9000
added
auto-merge-without-approval
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Nov 10, 2021
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 a commit
that referenced
this pull request
Nov 11, 2021
Now that we have #79394 and #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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This slightly moves the routing calculation out of a difficult to reason
about
switchstatement and into real OO method implementation. Its atiny tiny change but it makes me feel much better about it.