Async Search: correct shards counting #55758
Conversation
Async search allows users to retrieve partial results for a running search. For partial results, the number of successful shards does not include the skipped shards, while the response returned to users should. When a fatal failure happened after some partial results were processed, for instance because all of the shards failed to fetch their hits, the number of successful shards should be reset to align it with what ordinary search would return. Also, we recently had a bug where async search would miss tracking shard failures, which would have been caught if we had assertions in place that verified that whenever we get the last response, the number of failures included in it is the same as the failures that were tracked through the listener notifications.
javanna
added
>bug
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
| //We may have already received some partial results, in which case the number of successful shards reflects that despite the search | ||
| //has failed entirely at a later stage. We should consider all shards as failed given that none of them was able to e.g. fetch | ||
| //skipped shards are considered successful though | ||
| this.successfulShards = this.skippedShards; |
There was a problem hiding this comment.
I believe this makes what gets returned more consistent with what _search does, though we lose information on how many shards the partial results come from. We possibly need to expand the info that we return if we want to better represent this scenario where there are partial results yet the whole search has failed hence stopped.
There was a problem hiding this comment.
Why do we need to update these informations ? They are not used anymore when the response is final. I think #55683 has the more straightforward approach of keeping the final response as is.
There was a problem hiding this comment.
I think that this is the reason why you can end up with situations like successful: 3 failed: 3 total: 3 .
There was a problem hiding this comment.
I would rather want successful: 0 failed: 3 total:3 ( I left skipped out for simplicity)
I have yet to look at the linked PR, will do
There was a problem hiding this comment.
ok I am now up-to-date, I agree with you on updateFinalResponse, no need to touch successful shards anymore, but you commented on updateWithFailure which still has the problem of returning a shards header that search does not return? Hence shall we signal how many shards have returned partial results although all of them later failed, or shall we just zero the successful shards that makes things more consistent with search? Otherwise I think we would need to add more details on which phase has failed to the response to be more accurate... I will update and merge conflicts.
There was a problem hiding this comment.
I think we'll solve this discrepancy when wee add partial top hits in the response. In the meantime I don't think we should reset the successful shards, it would be weird to have successful: 0 (assuming no shards were skipped) but some partial results in the response ?
There was a problem hiding this comment.
I think it's weird either way. I cant make up my mind on which way is less weird :) I will revert this bit then and maybe add a comment that explains what happens today.
There was a problem hiding this comment.
maybe one thing we could rather do in this case is to reset shard failures, as they have caused a fatal failure and don't need to be returned as part of the search response too, they are already in the outer error section. That way we would have failed: 0 in the inner search response, which makes more sense as it's a snapshot of the results before the failure happened. Not too sure though if this may end up causing problems in other scenarios. Doing nothing is also fine with me, as long as we are aware that the shards section can be weird at times.
There was a problem hiding this comment.
What about ignoring fetch shard failures when building partial responses ?
We never return partial top hits if the response is partial so the successful and failure counts should only reflect the query phase ? This will change when we add the support for partial top hits but that's the least confusing solution I can think of.
| @@ -387,7 +387,7 @@ public void onFinalReduce(List<SearchShard> shards, TotalHits totalHits, Interna | |||
|
|
|||
| @Override | |||
| public void onResponse(SearchResponse response) { | |||
| searchResponse.get().updateFinalResponse(response.getSuccessfulShards(), response.getInternalResponse()); | |||
| searchResponse.get().updateFinalResponse(response); | |||
There was a problem hiding this comment.
+1, #55683 has the same change
There was a problem hiding this comment.
When I was working on mine I was wondering if I should assert that the final response looks "right" based on our updates. I never did that, but maybe it is good?
There was a problem hiding this comment.
what do you mean by "right" here?
There was a problem hiding this comment.
I'm not sure! That was part of why I didn't do it....
I was thinking that it might be useful to know if the response that we got here "agrees" with the results we got from the listener. Maybe that is just checking counts or something.
There was a problem hiding this comment.
You've already added the assertions below. Ignore me!
There was a problem hiding this comment.
I see, yea that is the reason why I added the whole response as argument ;)
| //We may have already received some partial results, in which case the number of successful shards reflects that despite the search | ||
| //has failed entirely at a later stage. We should consider all shards as failed given that none of them was able to e.g. fetch | ||
| //skipped shards are considered successful though | ||
| this.successfulShards = this.skippedShards; |
There was a problem hiding this comment.
Why do we need to update these informations ? They are not used anymore when the response is final. I think #55683 has the more straightforward approach of keeping the final response as is.
|
run elasticsearch-ci/default-distro |
Async search allows users to retrieve partial results for a running search. For partial results, the number of successful shards does not include the skipped shards, while the response returned to users should. Also, we recently had a bug where async search would miss tracking shard failures, which would have been caught if we had assertions in place that verified that whenever we get the last response, the number of failures included in it is the same as the failures that were tracked through the listener notifications.
Async search allows users to retrieve partial results for a running search. For partial results, the number of successful shards does not include the skipped shards, while the response returned to users should. Also, we recently had a bug where async search would miss tracking shard failures, which would have been caught if we had assertions in place that verified that whenever we get the last response, the number of failures included in it is the same as the failures that were tracked through the listener notifications.
Async search allows users to retrieve partial results for a running search. For partial results, the number of successful shards does not include the skipped shards, while the response returned to users should. Also, we recently had a bug where async search would miss tracking shard failures, which would have been caught if we had assertions in place that verified that whenever we get the last response, the number of failures included in it is the same as the failures that were tracked through the listener notifications.
Async search allows users to retrieve partial results for a running search. For partial results, the number of successful shards does not include the skipped shards, while the response returned to users should.
Also, we recently had a bug where async search would miss tracking shard failures, which would have been caught if we had assertions in place that verified that whenever we get the last response, the number of failures included in it is the same as the failures that were tracked through the listener notifications.