Move required routing validation to IndexRouting #79472
Merged
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
|
Pinging @elastic/es-distributed (Team:Distributed) |
This was referenced Oct 19, 2021
Now that we have a place responsible for picking shards we can move the "routing is required" validation there, right next to the validation for the `routing_path` stuff. This feels like a more convenient place to have it. Certainly a nice place to unit test it.
|
run elasticsearch-ci/part-1 |
henningandersen
approved these changes
Oct 26, 2021
| } | ||
| String concreteSingleIndex = indexNameExpressionResolver.concreteSingleIndex(clusterState, termVectorsRequest).getName(); | ||
| shardId = clusterService.operationRouting().shardId(clusterState, concreteSingleIndex, | ||
| termVectorsRequest.id(), termVectorsRequest.routing()); | ||
| } catch (Exception e) { | ||
| responses.set(i, new MultiTermVectorsItemResponse(null, | ||
| new MultiTermVectorsResponse.Failure(termVectorsRequest.index(), termVectorsRequest.id(), e))); |
There was a problem hiding this comment.
I think this changes the way we report a missing routing to list the original request index (which could be an alias) rather than the concrete index. I think we want to report the concrete index here?
Same in TransportMultiGetAction.
I suppose one could argue that reporting the original index-name is just as good since the info is baked into the exception message. But it does open a discussion on this being a breaking change (though a very small one)?
There was a problem hiding this comment.
But it does open a discussion on this being a breaking change (though a very small one)?
I don't think we consider error messages to be part of the semver contract. But changing it without thinking about it is not good. I'll have a closer look.
We fail to build the exception in 7.x. Big yikes.
…uting_from_source_clean_2
…uting_from_source_clean_2
|
@elasticmachine update branch |
0d1363d to
673d8bf
Compare
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.
Now that we have a place responsible for picking shards we can move the
"routing is required" validation there, right next to the validation for
the
routing_pathstuff. This feels like a more convenient place tohave it. Certainly a nice place to unit test it.