Skip to content
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

Return partial failures if search was cancelled. #64382

Merged
merged 5 commits into from
Nov 24, 2020

Conversation

jtibshirani
Copy link
Contributor

In #63520, we started cancelling searches that encounter shard failures and
don't allow partial results. In this case we return an 'all shards failed'
response, since there are no successful responses.

This PR proposes to return a 'partial shards failure' instead. The reasoning:
it's misleading to claim that 'all shards failed' when we only know at least
one shard failed (and preemptively cancelled the rest).

Addresses #64367.

@jtibshirani jtibshirani added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.11.0 labels Oct 29, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@jtibshirani
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

* search phase as "all shards failed". Note that if we cancelled the rest of the search request because of a
* partial failure, we don't count this as if "all shards failed".
*/
if (successfulOps.get() == 0 && requestCancelled.get() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by comment:

I wonder if we need to check if (shardFailures.size() == total) and if so, still report all shards failed? At least it will be a little bit confusing to get the partial shards failure exception if you only search one shard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point, that's a more robust check! I had assumed we wouldn't set requestCancelled unless there were multiple shards.

Copy link
Contributor Author

@jtibshirani jtibshirani Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed an update to only check shardSearchFailures.length == getNumShards(). But I think this only makes sense with #64337 -- I'll pull that in once it's merged and check if there are still test failures.

@jtibshirani
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jtibshirani jtibshirani merged commit 0604157 into elastic:master Nov 24, 2020
@jtibshirani jtibshirani deleted the search-failures branch November 24, 2020 20:54
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Nov 24, 2020
In elastic#63520, we started cancelling searches that encounter shard failures and
don't allow partial results. In this case we return an 'all shards failed'
response, since there are no successful responses.

This PR proposes to return a 'partial shards failure' instead. The reasoning:
it's misleading to claim that 'all shards failed' when we only know at least
one shard failed (and preemptively cancelled the rest).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants