Don't fail retention lease sync actions due to capacity constraints #109414
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
nicktindall
force-pushed
the
fix/105926_do_not_trip_circuit_breakers_retention_lease_sync
branch
from
ccb95e9
to
795287b
Compare
nicktindall
commented
Jun 18, 2024
...r/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java
Outdated
Show resolved
Hide resolved
nicktindall
commented
Jun 18, 2024
...r/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java
Outdated
Show resolved
Hide resolved
nicktindall
commented
Jun 18, 2024
server/src/main/java/org/elasticsearch/action/support/replication/TransportWriteAction.java
Outdated
Show resolved
Hide resolved
nicktindall
commented
Jun 18, 2024
server/src/main/java/org/elasticsearch/common/util/ArrayUtils.java
Outdated
Show resolved
Hide resolved
nicktindall
commented
Jun 18, 2024
server/src/main/java/org/elasticsearch/index/seqno/GlobalCheckpointSyncAction.java
Outdated
Show resolved
Hide resolved
nicktindall
commented
Jun 19, 2024
server/src/main/java/org/elasticsearch/common/util/ArrayUtils.java
Outdated
Show resolved
Hide resolved
…p_circuit_breakers_retention_lease_sync
nicktindall
added
>bug
:Distributed/CRUD
|
Hi @nicktindall, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed (Team:Distributed) |
nicktindall
added a commit
to nicktindall/elasticsearch
that referenced
this pull request
Jul 18, 2024
nicktindall
added a commit
that referenced
this pull request
Jul 18, 2024
DaveCTurner
reviewed
Jul 18, 2024
...r/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java
Outdated
Show resolved
Hide resolved
nicktindall
added a commit
to nicktindall/elasticsearch
that referenced
this pull request
Jul 18, 2024
ioanatia
pushed a commit
to ioanatia/elasticsearch
that referenced
this pull request
Jul 22, 2024
salvatore-campagna
pushed a commit
to salvatore-campagna/elasticsearch
that referenced
this pull request
Jul 23, 2024
…ntion_lease_sync # Conflicts: # server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseBackgroundSyncAction.java # server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java
nicktindall
changed the title
nicktindall
commented
Jul 30, 2024
| @@ -91,10 +91,10 @@ public RetentionLeaseSyncAction( | |||
| RetentionLeaseSyncAction.Request::new, | |||
| RetentionLeaseSyncAction.Request::new, | |||
| new ManagementOnlyExecutorFunction(threadPool), | |||
| PrimaryActionExecution.RejectOnOverload, | |||
| PrimaryActionExecution.Force, | |||
There was a problem hiding this comment.
I don't believe this change is necessary on the RetentionLeaseBackgroundSyncAction because it's not a TransportWriteAction, and therefore checkPrimaryLimits (where the indexing pressure check is done) is a no-op.
There was a problem hiding this comment.
I wonder if we even need to make any changes to RetentionLeaseBackgroundSyncAction. It doesn't appear that its failure will have the same consequences as that of RetentionLeaseSyncAction.
nicktindall
commented
Jul 30, 2024
…p_circuit_breakers_retention_lease_sync
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.
Closes #105926
RetentionLeaseSyncActionreplicates the retention leases from the primary node to the replicas and persists them to disk on both the primary and replicas.RetentionLeaseSyncActionis aTransportWriteAction. ATransportWriteActionexecutes an update on the primary copy of the shard then on the replica(s). If the primary operation succeeds, but the replica operation(s) do not, the shard is marked as failed and an expensive recovery process is kicked off.One of the reasons a
TransportActioncan fail is due to the circuit breakers residing on each node. If a request comes in and the node is very busy it can fail an action to prevent running out of memory.Because the
RetentionLeaseSyncActionconsumes a negligible amount of memory, this PR excludes the replica component of theRetentionLeaseSyncActionandBackgroundRetentionLeaseSyncActionfrom the circuit breaker checks. This will prevent a costly failure without significantly increasing the risk of out-of-memory errors.Edit: It has also become apparent that the indexing pressure check can fail the retention lease sync. I've included an additional change in this PR to force execution of the action on the primary to bypass such checks. Indexing pressure checks are already bypassed on the replica by default in
TransportReplicationAction