ReplicationOperation should fail gracefully #115341
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
Problem: finishAsFailed could be called asynchronously in the middle of operations like runPostReplicationActions which try to sync the translog. finishAsFailed immediately triggers the failure of the resultListener which releases the index shard primary operation permit. This means that runPostReplicationActions may try to sync the translog without an operation permit. Solution: We refactor the infrastructure of ReplicationOperation regarding pendingActions and the resultListener, by replacing them with a RefCountingListener. This way, if there are async failures, they are aggregated, and the result listener is called once, after all mid-way operations are done. For the specific error we got in issue elastic#97183, this means that a call to onNoLongerPrimary (which can happen if we fail to fail a replica shard or mark it as stale) will not immediately release the primary operation permit and the assertion in the translog sync will be honored. Fixes elastic#97183
kingherc
added
:Distributed Indexing/CRUD
elasticsearchmachine
added
the
needs:risk
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
I should be able to dig into review this tomorrow. |
Tim-Brooks
approved these changes
Oct 30, 2024
server/src/main/java/org/elasticsearch/action/support/replication/ReplicationOperation.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/support/replication/ReplicationOperation.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/support/replication/ReplicationOperation.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/support/replication/ReplicationOperation.java
Outdated
Show resolved
Hide resolved
Such as moving RefCountingListener to try with resources, and adding ActionListener.run in a couple of places.
|
Thanks @Tim-Brooks ! Incorporated your feedback, and also made some nit changes to ensure no exception escapes through (by making use of try-with-resources for the ActionListenerRef and some ActionListener.runs in a couple of places). Feel free to give another quick look if you'd like. |
LGTM |
kingherc
added a commit
to kingherc/elasticsearch
that referenced
this pull request
Nov 1, 2024
We introduce ActionListener.run() in order to ensure the RefCountingListener introduced by PR elastic#115341 , is the single point that is failed upon exceptions, and no exception escapes through the ReplicationOperation.execute() method. Fixes elastic#116071
This was referenced Nov 1, 2024
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Nov 4, 2024
Problem: finishAsFailed could be called asynchronously in the middle of operations like runPostReplicationActions which try to sync the translog. finishAsFailed immediately triggers the failure of the resultListener which releases the index shard primary operation permit. This means that runPostReplicationActions may try to sync the translog without an operation permit. Solution: We refactor the infrastructure of ReplicationOperation regarding pendingActions and the resultListener, by replacing them with a RefCountingListener. This way, if there are async failures, they are aggregated, and the result listener is called once, after all mid-way operations are done. For the specific error we got in issue elastic#97183, this means that a call to onNoLongerPrimary (which can happen if we fail to fail a replica shard or mark it as stale) will not immediately release the primary operation permit and the assertion in the translog sync will be honored. Fixes elastic#97183
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Nov 4, 2024
We introduce ActionListener.run() in order to ensure the RefCountingListener introduced by PR elastic#115341 , is the single point that is failed upon exceptions, and no exception escapes through the ReplicationOperation.execute() method. Fixes elastic#116071 Fixes elastic#116081 Fixes elastic#116073
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.
Problem:
finishAsFailed could be called before, or even asynchronously in the middle of, operations like runPostReplicationActions which try to sync the translog. finishAsFailed immediately triggers the failure of the resultListener which releases the index shard primary operation permit. This means that runPostReplicationActions may try to sync the translog without an operation permit.
Solution:
We refactor the infrastructure of ReplicationOperation regarding pendingActions and the resultListener, by replacing them with a RefCountingListener. This way, if there are failures, the result listener is called once, after all pending actions are done.
For the specific error we got in issue #97183, this means that a call to onNoLongerPrimary (which can happen if we fail to fail a replica shard or mark it as stale) will not immediately release the primary operation permit and the assertion in the translog sync will be honored.
Fixes #97183