Ensure error handler is called during SLM retention callback failure #55252
Conversation
This file contains 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
When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves elastic#55217
dakrone
added
>bug
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-features (:Core/Features/ILM+SLM) |
|
@elasticmachine update branch |
andreidan
reviewed
Apr 16, 2020
|
|
||
| @Override | ||
| public void onFailure(Exception e) { | ||
| fail("we have another err handler that should have been called"); |
There was a problem hiding this comment.
I found this confusing, but I see it's just how getAllRetainableSnapshots does the listener callback, so maybe it's a discussion we should have on a separate ocasion
There was a problem hiding this comment.
Yeah, we can do that maybe in future work since it's technically not really a bugfix.
andreidan
approved these changes
Apr 16, 2020
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Apr 16, 2020
…lastic#55252) When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves elastic#55217
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Apr 16, 2020
…lastic#55252) When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves elastic#55217
dakrone
added a commit
that referenced
this pull request
Apr 16, 2020
…55252) (#55321) When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves #55217
dakrone
added a commit
that referenced
this pull request
Apr 16, 2020
…55252) (#55324) When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves #55217
yyff
pushed a commit
to yyff/elasticsearch
that referenced
this pull request
Apr 17, 2020
…lastic#55252) When retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for the body of the `ActionListener`'s `onResponse` method to throw an Exception. In this case, the `errHandler` passed in may not be executed, resulting in the `running` boolean not being reset back to false. This commit uses `ActionListener.wrap(...)` instead of creating a new ActionListener, which ensures that if the `onResponse` fails in any way, the `onFailure` handler is still called. Resolves elastic#55217
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 retrieving the snapshots for a set of repos or deleting a single snapshot, it's possible for
the body of the
ActionListener'sonResponsemethod to throw an Exception. In this case, theerrHandlerpassed in may not be executed, resulting in therunningboolean not being reset backto false.
This commit uses
ActionListener.wrap(...)instead of creating a new ActionListener, which ensuresthat if the
onResponsefails in any way, theonFailurehandler is still called.Resolves #55217