Do not use split shards for search and refresh if they are not ready #132042
Conversation
elasticsearchmachine
added
v9.2.0
serverless-linked
lkts
force-pushed
the
filter_not_ready_split_shards_from_search
branch
from
6c1ee55 to
4f9584f
Compare
| for (int i = 0; i < indexRoutingTable.size(); i++) { | ||
| shardIds.add(indexRoutingTable.shard(i).shardId()); | ||
| } | ||
| Iterator<IndexShardRoutingTable> iterator = operationRouting.allWritableShards(projectState, index); |
There was a problem hiding this comment.
This handles refresh and flush APIs.
lkts
added
>non-issue
:Distributed Indexing/Distributed
elasticsearchmachine
added
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Would it be possible to unit test these routing changes at the level of |
| @@ -99,7 +101,8 @@ public void accept(ActionListener<Response> listener) { | |||
|
|
|||
| final ClusterState clusterState = clusterService.state(); | |||
| final ProjectMetadata project = projectResolver.getProjectMetadata(clusterState); | |||
There was a problem hiding this comment.
nit: I don't know how expensive a projectResolver is but perhaps it's a little cheaper to resolve once and then retrieve ProjectMetadata from projectState.metadata() ?
| * Returns an iterator of shards of the index that are ready to execute write requests. | ||
| * A shard may not be ready to execute these operations during processes like resharding. | ||
| */ | ||
| private static Iterator<IndexShardRoutingTable> allShardsReadyForWrites(ProjectState projectState, String index) { |
There was a problem hiding this comment.
I'm struggling a bit with naming. I worry that Ready could be confused with a shard that's in recovery. Such a shard might refuse operations but operations should be routed there anyway, because it's the owning shard of a request. Maybe allWriteAddressableShards?
| @@ -125,7 +132,7 @@ private static Set<IndexShardRoutingTable> computeTargetedShards( | |||
| // we use set here and not list since we might get duplicates | |||
| final Set<IndexShardRoutingTable> set = new HashSet<>(); | |||
| if (routing == null || routing.isEmpty()) { | |||
| collectTargetShardsNoRouting(projectState.routingTable(), concreteIndices, set); | |||
| collectTargetShardsNoRouting(projectState, concreteIndices, set); | |||
| } else { | |||
| collectTargetShardsWithRouting(projectState, concreteIndices, routing, set); | |||
There was a problem hiding this comment.
how is this branch resolving target shards to source shards before split?
There was a problem hiding this comment.
Is the routing passed in here based on what we got from IndexRouting ?
There was a problem hiding this comment.
This is handled inside IndexRouting#collectSearchShards, yes.
There was a problem hiding this comment.
Ok, but what about this codepath in collectTargetShardsWithRouting. When is this called and why does this not have to check for resharding ?
else {
for (int i = 0; i < indexRoutingTable.size(); i++) {
set.add(indexRoutingTable.shard(i));
}
There was a problem hiding this comment.
Yes, it should, thanks for catching that.
| return allShardsExceptSplitTargetsInStateBefore(projectState, index, IndexReshardingState.Split.TargetShardState.HANDOFF); | ||
| } | ||
|
|
||
| private static Iterator<IndexShardRoutingTable> allShardsExceptSplitTargetsInStateBefore( |
There was a problem hiding this comment.
Can we add a comment here so the reader knows it is associated with resharding and not to be confused with the split API.
There was a problem hiding this comment.
I think that class level documentation on IndexReshardingMetadata can be fairly easily discovered from this code and it explains what it is.
|
So this PR does not handle GETs, is that correct ? Can you add a description to this PR to explain which codepaths are covered. Looks like you are covering search, flush and refresh ? |
|
@ankikuma yes, GET is not handled |
|
Left one comment about |
This PR adjusts routing logic for search, refresh and flush APIs in order to take into account that some shards may be not usable for such operations based on the state of resharding metadata of the index. Note that GET is out of scope of this PR (index, update and delete operations were previously covered).