Async Search: correct shards counting #55758
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,7 +91,8 @@ synchronized void updatePartialResponse(int successfulShards, SearchResponseSect | |
| throw new IllegalStateException("received partial response out of order: " | ||
| + newSections.getNumReducePhases() + " < " + sections.getNumReducePhases()); | ||
| } | ||
| //when we get partial results skipped shards are not included in the provided number of successful shards | ||
| this.successfulShards = successfulShards + skippedShards; | ||
| this.sections = newSections; | ||
| this.isPartial = true; | ||
| this.isFinalReduce = isFinalReduce; | ||
|
|
@@ -101,12 +102,20 @@ synchronized void updatePartialResponse(int successfulShards, SearchResponseSect | |
| * Updates the response with the final {@link SearchResponseSections} merged from #<code>successfulShards</code> | ||
| * shards. | ||
| */ | ||
| synchronized void updateFinalResponse(SearchResponse searchResponse) { | ||
| failIfFrozen(); | ||
| assert searchResponse.getTotalShards() == totalShards : "received number of total shards differs from the one " + | ||
| "notified through onListShards"; | ||
| assert searchResponse.getSkippedShards() == skippedShards : "received number of skipped shards differs from the one " + | ||
| "notified through onListShards"; | ||
| assert searchResponse.getFailedShards() == buildShardFailures().length : "number of tracked failures differs from failed shards"; | ||
| // copy the response headers from the current context | ||
| this.responseHeaders = threadContext.getResponseHeaders(); | ||
| //we take successful from the final response, which overrides whatever value we set when we received the last partial results. | ||
| //This is important for cases where e.g. aggs work fine and then fetch fails on some of the shards but not all. | ||
| //The shards where fetch has failed should not counted as successful. | ||
| this.successfulShards = searchResponse.getSuccessfulShards(); | ||
| this.sections = searchResponse.getInternalResponse(); | ||
| this.isPartial = false; | ||
| this.isFinalReduce = true; | ||
| this.frozen = true; | ||
|
|
@@ -120,6 +129,10 @@ synchronized void updateWithFailure(Exception exc) { | |
| failIfFrozen(); | ||
| // copy the response headers from the current context | ||
| this.responseHeaders = threadContext.getResponseHeaders(); | ||
| //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 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 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 ? |
||
| this.isPartial = true; | ||
| this.failure = ElasticsearchException.guessRootCauses(exc)[0]; | ||
| this.frozen = true; | ||
|
|
||
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 ;)