[segment replication] Fix segment replication infinite retry due to stale metadata checkpoint - #20551
Conversation
Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThe PR introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant Service as SegmentReplicationTargetService
participant Replicator as SegmentReplicator
participant Target as SegmentReplicationTarget
participant Checkpoint as ReplicationCheckpoint Validator
Note over Service: onNewCheckpoint(checkpoint, shard, isRetry=true)
Service->>Service: processLatestReceivedCheckpoint(shard, thread, isRetry=true)
Service->>Service: onNewCheckpoint(checkpoint, shard, isRetry=true)
Service->>Replicator: startReplication(shard, checkpoint, source, isRetry=true, listener)
Replicator->>Target: new SegmentReplicationTarget(..., isRetry=true, listener)
Replicator->>Target: startReplication()
Target->>Checkpoint: validate checkpoint (isRetry=true)
alt isRetry=true
Checkpoint-->>Target: Bypass stale-check rejection
Target-->>Service: Proceed with retry replication
else isRetry=false
Checkpoint-->>Target: Apply full validation (may reject stale)
Target-->>Service: Reject or proceed based on validation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java (1)
139-142:⚠️ Potential issue | 🟠 Major
retryCopy()should create target withisRetry=true.The
retryCopy()method creates a newSegmentReplicationTargetusing the 4-argument constructor which defaultsisRetrytofalse. However, since this method is specifically called during retry scenarios, the new target should haveisRetry=trueto bypass stale checkpoint validation instartReplication.🐛 Proposed fix
`@Override` public SegmentReplicationTarget retryCopy() { - return new SegmentReplicationTarget(indexShard, checkpoint, source, listener); + return new SegmentReplicationTarget(indexShard, checkpoint, source, true, listener); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.javaserver/src/main/java/org/opensearch/indices/replication/AbstractSegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/MergedSegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.
Applied to files:
server/src/main/java/org/opensearch/indices/replication/AbstractSegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/MergedSegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicator.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.javaserver/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.java
🧬 Code graph analysis (1)
server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java (2)
server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.java (1)
Actions(85-89)server/src/main/java/org/opensearch/action/support/replication/TransportReplicationAction.java (1)
TransportReplicationAction(113-1689)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: detect-breaking-change
🔇 Additional comments (12)
server/src/main/java/org/opensearch/indices/replication/MergedSegmentReplicationTarget.java (1)
36-36: LGTM!The constructor correctly passes
isRetry=falseto the superclass, which is appropriate since merged segment replication targets are not subject to the same retry semantics as regular segment replication.server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java (1)
45-56: LGTM!The new constructor correctly accepts the
isRetryparameter and passes it to the superclass. The existing constructor delegation pattern maintains backward compatibility.server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java (1)
146-219: Good integration test for retry recovery scenario.The test properly validates that segment replication recovers from a
CircuitBreakingExceptionby:
- Setting up controlled failure injection using
MockTransportService- Using latches to ensure proper sequencing of the exception and subsequent checkpoint
- Verifying that documents eventually become searchable on both nodes
However, the test doesn't explicitly verify that the
isRetryflag is being set correctly during the retry path. Consider adding an assertion or logging to confirm the retry mechanism is working as intended.server/src/main/java/org/opensearch/indices/replication/AbstractSegmentReplicationTarget.java (2)
50-50: LGTM!The
isRetryfield is appropriately declared asprotected finalto allow subclass access while ensuring immutability.
174-177: Core fix correctly implemented.The condition now properly bypasses stale checkpoint validation when
isRetry=true. This allows retries to proceed even when the primary has cleared residual segment replication state (which was causing the infinite retry loop in issue#20550).The boolean expression is clear and correctly combines all conditions:
isSegRepLocalEnabled()- feature checkcheckpoint.isAheadOf(getMetadataCheckpoint)- stale checkpoint detectionfalse == isRecovering- recovery path bypassfalse == isRetry- retry path bypass (new)server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java (2)
114-124: LGTM!The
startReplicationmethod signature is correctly updated to includeisRetry, and the parameter is properly passed to theSegmentReplicationTargetconstructor. The Javadoc is also updated to document the new parameter.
77-99: LGTM!The default
startReplication(IndexShard)method correctly passesisRetry=falsefor initial replication attempts that are not retries.server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.java (5)
291-307: LGTM!The
onNewCheckpointmethod is correctly split into two overloads:
- A convenience method that defaults
isRetry=falsefor backward compatibility- The main implementation that accepts the
isRetryflagThis maintains a clean API while supporting the retry scenario.
344-344: LGTM!The
startReplicationcall correctly passes theisRetryflag through to the replicator, ensuring retry state is propagated throughout the replication flow.
376-379: Key fix: Retry path correctly setsisRetry=true.This is the critical fix for issue
#20550. When replication fails without requiring a shard failure, the retry is initiated withisRetry=true, which will bypass stale checkpoint validation inAbstractSegmentReplicationTarget.startReplication.
491-522: LGTM!The
processLatestReceivedCheckpointoverloads follow the same pattern asonNewCheckpoint:
- A convenience method defaulting
isRetry=false- The main implementation that accepts and propagates
isRetryThe
isRetryflag is correctly passed through toonNewCheckpointat line 510.
529-552: LGTM!The
startReplicationoverloads maintain backward compatibility while addingisRetrysupport. The parameter is correctly forwarded toreplicator.startReplication.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
❌ Gradle check result for 0a46006: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❗ AI-powered Code-Diff-Analyzer found issues on commit fd4ba88.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
❌ Gradle check result for fd4ba88: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 095718a: ABORTED Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❗ AI-powered Code-Diff-Analyzer found issues on commit 50ed2b5.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
|
❌ Gradle check result for ab70719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ab70719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
guojialiang92
added
the
skip-diff-analyzer
|
❌ Gradle check result for ab70719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ab70719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❕ Gradle check result for c193495: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20551 +/- ##
============================================
+ Coverage 73.25% 73.35% +0.09%
- Complexity 72103 72205 +102
============================================
Files 5798 5798
Lines 329732 329756 +24
Branches 47519 47524 +5
============================================
+ Hits 241554 241886 +332
+ Misses 68805 68500 -305
+ Partials 19373 19370 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tale metadata checkpoint (opensearch-project#20551) * reproduce stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * add change log Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> --------- Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
…tale metadata checkpoint (opensearch-project#20551) * reproduce stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * add change log Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> --------- Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
…tale metadata checkpoint (opensearch-project#20551) * reproduce stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix stale ckp exception Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * fix test Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> * add change log Signed-off-by: guojialiang <guojialiang.2012@bytedance.com> --------- Signed-off-by: guojialiang <guojialiang.2012@bytedance.com>
A segment replication retry can finalize against a stale metadata checkpoint returned by the primary (see #20550, #20551), leaving the replica behind the checkpoint it was asked to sync to. This leads to the replica being stale until the next publish. If another publish never happens, it is stale forever. I believe this is the case of flakiness in FullRollingRestartIT where the test fails on timeout waiting for the replica to catch up. The fix is to compare against the replica's achieved checkpoint instead so a round that finalized behind its target retriggers catch-up. In the normal case the achieved checkpoint matches the target, so no extra rounds are introduced. Signed-off-by: Andrew Ross <andrross@amazon.com>
2 participants
Description
This PR is to address the issues in #[20550].
In the case of failure retry, no ckp verification is performed. As long as the process enters Phase
GET_SEGMENT_FILESduring the retry, the primary shard will clear the residual segment replication information due to IndexShardClosedException caused by the replica.Related Issues
Resolves #[20550]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.