Fix Exception Handling for TransportShardBulkAction #41006
Merged
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
* Prior to elastic#39793 exceptions for the primary write and delete actions were bubbled up to the caller so that closed shards would be handled accordingly upstream. elastic#39793 accidentally changed the behaviour here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards. * This fix returns to that behaviour and adjusts the listeners passed in `TransportReplicationAction` such that they behave like the previous synchronous `catch`. * Dried up the exception handling slightly for that and inlined all the listeners to make the logic a little easier to follow * Reenable SplitIndexIT now that clsoed shards are properly handled again * Closes elastic#40944
original-brownbear
added
>non-issue
:Distributed/CRUD
|
Pinging @elastic/es-distributed |
dnhatn
approved these changes
Apr 9, 2019
There was a problem hiding this comment.
LGTM. Thanks @original-brownbear.
I hope we can keep "All exceptions should be handled by #executeBulkItemRequest" but I could not find a good way to achieve it.
|
thanks @dnhatn !
+1, let's see what the future brings :) |
gurkankaymak
pushed a commit
to gurkankaymak/elasticsearch
that referenced
this pull request
May 27, 2019
* Prior to elastic#39793 exceptions for the primary write and delete actions were bubbled up to the caller so that closed shards would be handled accordingly upstream. elastic#39793 accidentally changed the behaviour here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards. * This fix returns to that behaviour and adjusts the listeners passed in `TransportReplicationAction` such that they behave like the previous synchronous `catch`. * Dried up the exception handling slightly for that and inlined all the listeners to make the logic a little easier to follow * Reenable SplitIndexIT now that clsoed shards are properly handled again * Closes elastic#40944
gurkankaymak
pushed a commit
to gurkankaymak/elasticsearch
that referenced
this pull request
May 27, 2019
Fixed by elastic#41006 Closes elastic#40948
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.
were bubbled up to the caller so that closed shards would be handled accordingly upstream.
Make Transport Shard Bulk Action Async #39793 accidentally changed the behavior here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards.
TransportReplicationActionsuch that they behave like the previous synchronous
catch.easier to follow
marked non-issue since this bug didn't go into any release yet