SearchRequest#allowPartialSearchResults does not handle successful retries #43095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tries When set to false, allowPartialSearchResults option does not check if the shard failures have been reseted to null. The atomic array, that is used to record shard failures, is filled with a null value if a successful request on a shard happens after a failure on a shard of another replica. In this case the atomic array is not empty but contains only null values so this shouldn't be considered as a failure since all shards are successful (some replicas have failed but the retries on another replica succeeded). This change fixes this bug by checking the content of the atomic array and fails the request only if allowPartialSearchResults is set to false and at least one shard failure is not null. Closes elastic#40743
jimczi
added
>bug
:Search/Search
Collaborator
|
Pinging @elastic/es-search |
markharwood
reviewed
Jun 11, 2019
Contributor
markharwood
left a comment
markharwood
left a comment
There was a problem hiding this comment.
LGTM - test passing here.
Just some commented-out code in the test class that looks like it needs dealing with
| } | ||
| //} else { | ||
| // initializing.add(routing); | ||
| //} |
Contributor
There was a problem hiding this comment.
Comments need removing?
jimczi
added a commit
that referenced
this pull request
Jun 12, 2019
…tries (#43095) When set to false, allowPartialSearchResults option does not check if the shard failures have been reseted to null. The atomic array, that is used to record shard failures, is filled with a null value if a successful request on a shard happens after a failure on a shard of another replica. In this case the atomic array is not empty but contains only null values so this shouldn't be considered as a failure since all shards are successful (some replicas have failed but the retries on another replica succeeded). This change fixes this bug by checking the content of the atomic array and fails the request only if allowPartialSearchResults is set to false and at least one shard failure is not null. Closes #40743
jimczi
added a commit
that referenced
this pull request
Jun 12, 2019
…tries (#43095) When set to false, allowPartialSearchResults option does not check if the shard failures have been reseted to null. The atomic array, that is used to record shard failures, is filled with a null value if a successful request on a shard happens after a failure on a shard of another replica. In this case the atomic array is not empty but contains only null values so this shouldn't be considered as a failure since all shards are successful (some replicas have failed but the retries on another replica succeeded). This change fixes this bug by checking the content of the atomic array and fails the request only if allowPartialSearchResults is set to false and at least one shard failure is not null. Closes #40743
jimczi
added a commit
that referenced
this pull request
Jun 12, 2019
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Jun 12, 2020
Before elastic#57042 the max_buckets test would consistently pass because the request would consistently fail. In particular, the request would fail on the data node. After elastic#57042 it only fails on the coordinating node. When the max_buckets test is run in a mixed version cluster it consistently fails on *either* the data node or the coordinating node. Except when the coordinating node is missing elastic#43095. In that case if the one data node has elastic#57042 and one does not, *and* the one that doesn't gets the request first, fails it as expected, and then the coordinating node retries the request on the node with elastic#57042. When that happens the request fails mysteriously with "partial shard failures" as the error message but not partial failures reported. This is *exactly* the bug fixed in elastic#43095. This updates the test to be skipped in mixed version clusters without elastic#43095 because they *sometimes* fail the test spuriously. The request fails in those cases, just like we expect, but with a mysterious error message. Closes elastic#57657
nik9000
added a commit
that referenced
this pull request
Jun 12, 2020
Before #57042 the max_buckets test would consistently pass because the request would consistently fail. In particular, the request would fail on the data node. After #57042 it only fails on the coordinating node. When the max_buckets test is run in a mixed version cluster it consistently fails on *either* the data node or the coordinating node. Except when the coordinating node is missing #43095. In that case if the one data node has #57042 and one does not, *and* the one that doesn't gets the request first, fails it as expected, and then the coordinating node retries the request on the node with #57042. When that happens the request fails mysteriously with "partial shard failures" as the error message but not partial failures reported. This is *exactly* the bug fixed in #43095. This updates the test to be skipped in mixed version clusters without #43095 because they *sometimes* fail the test spuriously. The request fails in those cases, just like we expect, but with a mysterious error message. Closes #57657
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When set to false, allowPartialSearchResults option does not check if the
shard failures have been reseted to null. The atomic array, that is used to record
shard failures, is filled with a null value if a successful request on a shard happens
after a failure on a shard of another replica. In this case the atomic array is not empty
but contains only null values so this shouldn't be considered as a failure since all
shards are successful (some replicas have failed but the retries on another replica succeeded).
This change fixes this bug by checking the content of the atomic array and fails the request only
if allowPartialSearchResults is set to false and at least one shard failure is not null.
Closes #40743