Add support of IndexWarmer for replica shards with segment replication enabled - #20650
Conversation
|
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 Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces support for warming indexes during segment replication on replica shards by adding a WarmerRefreshListener to NRTReplicationEngine that triggers warmer invocation after refresh operations. The implementation includes core engine changes, comprehensive test coverage, and updates to test infrastructure to support warmer configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Kartik031299
changed the title
75086f4 to
57240bf
Compare
|
❌ Gradle check result for 57240bf: 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? |
57240bf to
3f98f86
Compare
|
❌ Gradle check result for 3f98f86: null 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? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (1)
928-944: Pre-existing:maybeRetentionLeasesSupplierparameter is silently overridden.Not introduced by this PR, but worth noting: the 10-parameter overload receives
maybeRetentionLeasesSupplier(line 925) but then passesmaybeGlobalCheckpointSupplier == null ? null : () -> RetentionLeases.EMPTY(line 940) instead, ignoring the caller-supplied value. This is a pre-existing issue — thewarmer: nulladdition at line 943 is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java` around lines 928 - 944, The config(...) call in EngineTestCase.java is incorrectly overriding the caller-supplied maybeRetentionLeasesSupplier by passing maybeGlobalCheckpointSupplier == null ? null : () -> RetentionLeases.EMPTY; change that argument to pass the actual maybeRetentionLeasesSupplier parameter (the 9th/10th argument) so the method uses the provided supplier rather than always returning RetentionLeases.EMPTY; verify the arguments align with config(...)'s parameter order (especially maybeGlobalCheckpointSupplier and maybeRetentionLeasesSupplier) and keep the added warmer=null as is.server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java (1)
577-593: Consider passingEngine.Warmerdirectly instead of the fullEngineConfig.The
WarmerRefreshListeneronly usesengineConfig.getWarmer()from the config. Passing just theWarmerwould make the dependency explicit and the class easier to reason about.Proposed refactor
- WarmerRefreshListener( - Logger logger, - AtomicBoolean isEngineClosed, - EngineConfig engineConfig, - NRTReplicationReaderManager readerManager - ) { - this.warmer = engineConfig.getWarmer(); + WarmerRefreshListener( + Logger logger, + AtomicBoolean isEngineClosed, + Engine.Warmer warmer, + NRTReplicationReaderManager readerManager + ) { + this.warmer = warmer;And at the call site (line 107):
- this.readerManager.addListener(new WarmerRefreshListener(logger, isClosed, engineConfig, this.readerManager)); + this.readerManager.addListener(new WarmerRefreshListener(logger, isClosed, engineConfig.getWarmer(), this.readerManager));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java` around lines 577 - 593, WarmerRefreshListener currently receives an EngineConfig only to call engineConfig.getWarmer(); change its constructor to accept Engine.Warmer directly and store that in the warmer field, update the WarmerRefreshListener instantiation sites to pass engineConfig.getWarmer() instead of the whole EngineConfig, and remove the unused EngineConfig parameter to make dependencies explicit (adjust the WarmerRefreshListener constructor signature and any call sites accordingly).modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.java (1)
126-160: Consider reusinggetWarmerTotalForShardType()helper to reduce duplication.The inline accumulation logic on lines 136–148 duplicates the
getWarmerTotalForShardType()helper defined at line 303. The second test (testWarmerInvokedOnReplicaAfterForceMerge) already uses the helper.♻️ Proposed refactor
assertBusy(() -> { IndicesStatsResponse statsResponse = client().admin() .indices() .prepareStats(INDEX_NAME) .clear() .setWarmer(true) .get(); - ShardStats[] shardStatsArray = statsResponse.getShards(); - - long primaryWarmerTotal = 0; - long replicaWarmerTotal = 0; - - for (ShardStats shardStats : shardStatsArray) { - WarmerStats warmerStats = shardStats.getStats().getWarmer(); - assertNotNull("Warmer stats should not be null", warmerStats); - - if (shardStats.getShardRouting().primary()) { - primaryWarmerTotal += warmerStats.total(); - } else { - replicaWarmerTotal += warmerStats.total(); - } - } + long primaryWarmerTotal = getWarmerTotalForShardType(statsResponse.getShards(), true); + long replicaWarmerTotal = getWarmerTotalForShardType(statsResponse.getShards(), false); assertThat( "Warmer should have been invoked on primary shard",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.java` around lines 126 - 160, The test duplicates logic for summing warmer totals across shards; replace the inline accumulation in the anonymous assertBusy block with calls to the existing helper getWarmerTotalForShardType() (use it to get primary and replica totals) so the test reuses that method and removes the loop and local primaryWarmerTotal/replicaWarmerTotal variables; keep the same assertions comparing the totals to greaterThan(0L) but obtain the totals via getWarmerTotalForShardType() to eliminate duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdmodules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.javaserver/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/test/java/org/opensearch/index/engine/NRTReplicationEngineTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.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/index/engine/NRTReplicationEngine.java
🔇 Additional comments (13)
CHANGELOG.md (1)
15-15: LGTM!Changelog entry is well-placed under the "Added" section and follows the existing format.
server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java (1)
105-107: Verify ordering: WarmerRefreshListener is added after internal refresh listeners.The warmer listener is registered after the loop that adds
engineConfig.getInternalRefreshListener()entries (lines 102-104). This means warming runs after other internal listeners. This mirrors the pattern inInternalEnginewhere warming also happens after internal listeners, so the ordering is consistent.One consideration: if this engine is closed before the constructor completes (i.e.,
successis set tofalseat line 140), thereaderManageris closed in thefinallyblock (line 145), so the listener won't fire on a partially-constructed engine. This is safe.test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (2)
862-889: Newconfigoverload withEngine.Warmerlooks correct.The overload properly creates default test objects (empty
EventListener,NoneCircuitBreakerService) and delegates to the terminal 12-parameter overload. The retention leases supplier logic is consistent with the existing pattern.
947-1022: Terminal config overload correctly wires warmer into EngineConfig.Builder.The new parameters (
eventListener,warmer) are properly threaded through to the builder at lines 1002 and 1008. The rest of the builder chain is unchanged.server/src/test/java/org/opensearch/index/engine/NRTReplicationEngineTests.java (3)
411-449: Clean overload chain forbuildNrtReplicaEnginewith optionalWarmer.The delegation chain is straightforward: 2-param → 4-param (null warmer), 3-param → 4-param (null warmer). The 4-param overload correctly passes the warmer through
config(...). Existing tests are unaffected since they all go through the null-warmer path.
761-786: Good resilience test — confirms warmer exceptions don't disrupt segment updates.The test validates an important invariant: a failing warmer must not prevent
updateSegmentsfrom completing successfully. The assertion at line 784 confirms the engine's segment state is consistent after a warmer failure.
719-759: Good test coverage for warmer invocation onupdateSegments.The test correctly validates that the warmer is invoked once per
updateSegmentscall that results in a refresh with changed segments. The second assertion after force-merge confirms warming also fires on subsequent segment updates. The use ofgetFirst()on line 748 is consistent with established patterns throughout the OpenSearch codebase.server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (5)
3480-3481: LGTM — updated config wiring for cleanup-enabled merge failure test.The new arguments align with the expanded
EngineConfigsignature.
3582-3583: LGTM — index-sort cleanup test matches new config signature.The added parameters look consistent with the new constructor shape.
3677-3678: LGTM — cleanup-disabled test config updated appropriately.The updated argument list looks correct.
3775-3776: LGTM — failing-directory cleanup test config updated appropriately.No issues with the updated parameter list.
7223-7234: LGTM — min-retained-seqno test config now passes retention leases + circuit breaker.This matches the expanded
EngineConfigconstructor usage in the rest of the tests.modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.java (1)
1-50: Well-structured integration test with good coverage of the warming-on-replica feature.The test class covers the key scenarios: eager global ordinals on replica after segment replication, warming after force merge, and the case without eager global ordinals. The helper methods (
waitForSearchableDocs,getWarmerTotalForShardType) are clean and well-factored. The cluster scope (TEST) withnumDataNodes = 0gives precise control over node startup order, which is appropriate for these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.java`:
- Around line 278-289: The current assertions using greaterThanOrEqualTo(0L) for
WarmerStats.total() (variables primaryTotal and replicaTotal) are vacuous; to
actually verify warmers ran change the assertions to assertThat(...,
greaterThan(0L)) for both primaryTotal and replicaTotal (or, if you intended to
allow zero, update the assertion messages to state zero is acceptable and remove
the misleading "should have been invoked" wording); keep waitForSearchableDocs
as-is if you're only verifying segment replication succeeded.
- Around line 202-210: The two separate calls to
client().admin().indices().prepareStats(INDEX_NAME)... create a TOCTOU race
causing replicaWarmerTotalBeforeMerge and primaryWarmerTotalBeforeMerge to be
based on different stats snapshots; replace the two calls with one stats
response variable (call prepareStats once for INDEX_NAME and store its result),
then pass that single stats response's shards to getWarmerTotalForShardType to
compute replicaWarmerTotalBeforeMerge and primaryWarmerTotalBeforeMerge so both
values come from the same snapshot; update the code references to use that
single stats response wherever getWarmerTotalForShardType is invoked.
- Around line 63-66: The `@Before` method setup in class
SegmentReplicationReplicaIndexWarmerIT is declared private which prevents JUnit4
from recognizing it; change the visibility of the setup method (annotated with
`@Before`) from private to public so JUnit can discover and run it and ensure
internalCluster().startClusterManagerOnlyNode() is executed before tests.
In `@server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java`:
- Around line 598-612: Move the call to readerManager.acquire() inside the try
so any IOException or other exception from acquire is caught and treated as
best-effort warming; specifically, in NRTReplicationEngine.afterRefresh(boolean)
wrap readerManager.acquire() and warmer.warm(reader) in the same try/catch and
catch Exception (or IOException) and call logger.warn("failed to warm reader
replica", e) only if isEngineClosed.get() is false; ensure
readerManager.release(reader) is only called in finally when a non-null reader
was successfully acquired.
---
Nitpick comments:
In
`@modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/SegmentReplicationReplicaIndexWarmerIT.java`:
- Around line 126-160: The test duplicates logic for summing warmer totals
across shards; replace the inline accumulation in the anonymous assertBusy block
with calls to the existing helper getWarmerTotalForShardType() (use it to get
primary and replica totals) so the test reuses that method and removes the loop
and local primaryWarmerTotal/replicaWarmerTotal variables; keep the same
assertions comparing the totals to greaterThan(0L) but obtain the totals via
getWarmerTotalForShardType() to eliminate duplication.
In `@server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java`:
- Around line 577-593: WarmerRefreshListener currently receives an EngineConfig
only to call engineConfig.getWarmer(); change its constructor to accept
Engine.Warmer directly and store that in the warmer field, update the
WarmerRefreshListener instantiation sites to pass engineConfig.getWarmer()
instead of the whole EngineConfig, and remove the unused EngineConfig parameter
to make dependencies explicit (adjust the WarmerRefreshListener constructor
signature and any call sites accordingly).
In
`@test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java`:
- Around line 928-944: The config(...) call in EngineTestCase.java is
incorrectly overriding the caller-supplied maybeRetentionLeasesSupplier by
passing maybeGlobalCheckpointSupplier == null ? null : () ->
RetentionLeases.EMPTY; change that argument to pass the actual
maybeRetentionLeasesSupplier parameter (the 9th/10th argument) so the method
uses the provided supplier rather than always returning RetentionLeases.EMPTY;
verify the arguments align with config(...)'s parameter order (especially
maybeGlobalCheckpointSupplier and maybeRetentionLeasesSupplier) and keep the
added warmer=null as is.
f6f7990 to
d3bc827
Compare
018dee8 to
1fe4b07
Compare
|
Persistent review updated to latest commit 1fe4b07 |
|
❌ Gradle check result for 1fe4b07: 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? |
1fe4b07 to
70b1f3d
Compare
|
Persistent review updated to latest commit 70b1f3d |
|
❌ Gradle check result for 70b1f3d: null 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? |
70b1f3d to
223256b
Compare
|
Persistent review updated to latest commit 223256b |
|
❌ Gradle check result for 223256b: null 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? |
223256b to
d4f89d7
Compare
|
Persistent review updated to latest commit d4f89d7 |
|
❌ Gradle check result for d4f89d7: 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? |
d4f89d7 to
7e22260
Compare
|
Persistent review updated to latest commit 7e22260 |
|
❕ Gradle check result for 7e22260: 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. |
…reader-manager Signed-off-by: Kartik Bansal <kbansal2@atlassian.com>
7e22260 to
fb51d06
Compare
|
Persistent review updated to latest commit fb51d06 |
…ards (opensearch-project#20650) Signed-off-by: Kartik Bansal <kbansal2@atlassian.com> Co-authored-by: Kartik Bansal <kbansal2@atlassian.com> Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
…ards (opensearch-project#20650) Signed-off-by: Kartik Bansal <kbansal2@atlassian.com> Co-authored-by: Kartik Bansal <kbansal2@atlassian.com> Signed-off-by: Aparajita Pandey <aparajita31pandey@gmail.com>
…ards (opensearch-project#20650) Signed-off-by: Kartik Bansal <kbansal2@atlassian.com> Co-authored-by: Kartik Bansal <kbansal2@atlassian.com>
4 participants
Description
Added WarmerRefreshListener to trigger Engine.Warmer on every refresh invoked for NRTReplicationReaderManager whenever segments are updates on replica shards after processing replication checkpoint
Related Issues
Bug: #20642
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.