Fix Incorrect Transport Response Handler Type #38264
Merged
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
* The response type here is not empty and was always wrong but this only became visible now that 0a604e3 was introduced * As a result of 0a604e3 we started actually handling the response of this request and logging/handling exceptions before that we simply dropped the classcast exception here quietly using the empty response handler * Closes #38226
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
Collaborator
|
Pinging @elastic/es-distributed |
dnhatn
approved these changes
Feb 2, 2019
Member
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
LGTM. Thanks @original-brownbear.
Contributor
Author
|
@dnhatn thanks for the quick review, bad news though: Looking into that now. I'd suggest if I can't fix it quickly I re-mute the tests and at least merge this fix to cut back on the noise? It seems like this could hit other tests that haven't yet failed too randomly. |
Contributor
Author
|
Fixed in 7b26929 The issue was that the changed busy assert block died on a normal exception when concurrently loading the repository data (I guess we could look into making the behaviour of the FS repository here nicer eventually): |
Contributor
Author
|
@dnhatn can you take another look please? :) |
Contributor
Author
|
Jenkins run elasticsearch-ci/2 |
original-brownbear
added
>test
dnhatn
approved these changes
Feb 2, 2019
Member
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
Thanks for an extra iteration @original-brownbear.
| client.admin().cluster().prepareSnapshotStatus("repository").setSnapshots("snap").get(); | ||
| assertThat(status.getSnapshots().iterator().next().getState(), equalTo(State.ABORTED)); | ||
| } catch (Exception e) { | ||
| throw new AssertionError(e); |
Member
There was a problem hiding this comment.
maybe add a comment to say that here we rethrow in AssertionError to force assertBusy to retry.
Contributor
Author
There was a problem hiding this comment.
Sure, I added it :)
Contributor
There was a problem hiding this comment.
@original-brownbear this assertBusy failed on a local ./gradlew check - here is the log from the test:
testAbortedSnapshotDuringInitDoesNotStart-failure.log.gz.
Contributor
Author
There was a problem hiding this comment.
@DaveCTurner thanks! I think I have an idea where this is coming from still ... Will fix later/tomorrow :)
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.
of this request and logging/handling exceptions before that we simply dropped the class cast exception here quietly using the empty response handler
>non-issuebecause this never made it into a release