Only aggregations require at least one shard request#115314
Only aggregations require at least one shard request#115314piergm merged 14 commits intoelastic:mainfrom
Conversation
piergm
added
>enhancement
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @piergm, I've created a changelog YAML for you. |
andreidan
left a comment
andreidan
left a comment
There was a problem hiding this comment.
Thanks for working on this Matteo.
Really nice PR description and fix 🚀
LGTM, assuming CI is happy
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
|
Why is this a 9.0 only change? Should we backport it? |
| public void testSortByFieldOneClusterHasNoResults() throws Exception { | ||
| assumeMultiClusterSetup(); | ||
| // set to a value greater than the number of shards to avoid differences due to the skipping of shards | ||
| // setting aggs to avoid differences due to the skipping of shards when matching none |
There was a problem hiding this comment.
this is interesting, the previous suggested we were skipping can match, that would have been a better work-around, but if your code change was required, that means that we were not skipping can match? was it outdated then?
There was a problem hiding this comment.
The previous comment was outdated, there was a refactor of this method a while back and the setPreFilterShardSize the comments referees to was removed
| .setSize(5), | ||
| response -> { | ||
| assertNull(response.getHits().getTotalHits()); | ||
| assertEquals(new TotalHits(0, TotalHits.Relation.EQUAL_TO), response.getHits().getTotalHits()); |
There was a problem hiding this comment.
Interesting, why was it null before? We were unskipping one shard, yet getting null total hits?
There was a problem hiding this comment.
It's because of SearchPhaseController#merge when we skip all the shards we have an empty result and therefore we return SearchResponseSections.EMPTY_WITH_TOTAL_HITS;. I wonder if this should instead be EMPTY_WITHOUT_TOTAL_HITS.
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left a couple of questions, this is a good cleanup!
Thanks for the review @javanna, my reasoning here is that this is not a bug therefore I wasn't sure if we need to backport it. Perhaps just to 8.x? |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* unskipping shards only when aggs * Update docs/changelog/115314.yaml * fixed more tests * null check for searchRequest.source() (cherry picked from commit 7f573c6)
|
Yes, we tend to backport changes to 8.x for now (next 8.x minor release), bugfixes only to upcoming patch releases. |
* unskipping shards only when aggs * Update docs/changelog/115314.yaml * fixed more tests * null check for searchRequest.source()
|
I wanted to backport this change but apparently the original PR that create the test issue never got merged to 8x (contrary to what the labeling says). #115794 |
…115794) * Only aggregations require at least one shard request (#115314) * unskipping shards only when aggs * Update docs/changelog/115314.yaml * fixed more tests * null check for searchRequest.source() (cherry picked from commit 7f573c6) * applying #115774 * skipped test * fixed test --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
|
Found the same inconsistency around backporting. I think that this change is mislabelled, I will relabel 8.18.0. |
5 participants
In the special case where we have no hits because the coordinator rewrote the query to match none we need to get at least one search response in order to produce a valid search result. This is true only if we have an aggregation in the query.
Only for aggs we need to contact the shard because we need the mappings for the queried index in order to produce a valid aggregation response, and mappings are only materialized in memory in data nodes where a certain index is allocated.
This pr updates this logic and sets
requireAtLeastOneMatchonly when searchRequest has aggregations.The updated logic will set searchResponse.skippedShards == searchResponse.successfulShards == totalShards when we can skip all the shards because we have no aggs.