Return true for can_match on idle search shards #55428
Conversation
dnhatn
added
>bug
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
|
@elasticmachine test this please |
++. I will update this PR. Thanks Jim. |
dnhatn
changed the title
dnhatn
changed the title
| @@ -1113,6 +1113,9 @@ public CanMatchResponse canMatch(ShardSearchRequest request) throws IOException | |||
| assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType(); | |||
| IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex()); | |||
| IndexShard indexShard = indexService.getShard(request.shardId().getId()); | |||
| if (indexShard.isSearchIdle()) { | |||
There was a problem hiding this comment.
Do we also need to check whether there are changes that have not been refreshed? Otherwise this change will have a terrible effiect on the warm tier by returning can_match=true up to once per second even though no changes are being made to indices? Or are we already protected against this?
There was a problem hiding this comment.
Good catch. I pushed 0e7f976.
There was a problem hiding this comment.
I wonder if we can still extract the min/max values of the primary sort since we don't need the values to be accurate and use it only to order the shards execution. If indices are in the warm tier they should contain some recent data that we'd like to visit first if the query is sorted by timestamp.
There was a problem hiding this comment.
This sounds potentially dangerous to me. I wouldn't think of the special case of search-idle indices if someone raised an idea that would leverage min/max values and rely on the fact that the max value to be an upper bound and the min value to be a lower bound?
There was a problem hiding this comment.
We don't have this guarantee today either, the reader of the query phase can be different from the one used in the can match phase. I can update the comments to make it more clear in the code that we don't create a point in time reader during the can match phase.
There was a problem hiding this comment.
I agree that the can_match phase might get a different reader from the query phase, but I'm less concerned by this since the reader we get in the can_match phase would also be a legal reader to run the query phase on, which isn't the case here? I don't feel strongly about this, ok to return the min/max values and +1 to document this.
There was a problem hiding this comment.
Right, I also thought about this because having these guarantees would mean that we can filter shards on the coord node based on these values. We don't do this today because of search-idle shards so we delay the skipping when verifying the values locally during the query phase.
The other option that was proposed by Nhat in this pr was to perform the refresh of search-idle shards during the can_match phase. It has the advantage of making all min/max values accurate for the search request but could make the phase much slower.
I don't feel strongly about this, ok to return the min/max values and +1 to document this.
++, thanks
There was a problem hiding this comment.
Thanks Adrien and Jim for the discussion here. I pushed 80f492f.
|
run elasticsearch-ci/1 |
With this change, we will always return true for can_match requests on idle search shards; otherwise, some shards will never get refreshed if all search requests perform the can_match phase (i.e., total shards > pre_filter_shard_size). Relates #27500 Relates #50043 Co-authored-by: Nhat Nguyen <nhat.nguyen@elastic.co>
With this change, we will always return true for can_match requests on idle search shards; otherwise, some shards will never get refreshed if all search requests perform the can_match phase (i.e., total shards > pre_filter_shard_size).
Relates #27500
Relates #50043