-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Async Search: correct shards counting #55758
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to check that the final response is consistent with informations we received during execution. However I think that #55683 simplifies the logic and should be adapted here.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, #55683 has the same change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by "right" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've already added the assertions below. Ignore me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.