Writable warm relocation recovery by nisgoel-amazon · Pull Request #14670 · opensearch-project/OpenSearch

nisgoel-amazon · 2024-07-08T10:32:33Z

Description

Changes are related to warm index recovery and relocation flow. In this change we are not downloading the complete files in replicas.

Related Issues

Resolves #13647

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Nishant Goel <nisgoel@amazon.com>

github-actions · 2024-07-08T11:17:14Z

❌ Gradle check result for 41fd1ec: 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?

github-actions · 2024-07-08T11:31:11Z

❌ Gradle check result for 3004994: 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?

github-actions · 2024-07-08T11:38:07Z

❌ Gradle check result for d42263b: 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?

mch2 · 2024-07-09T21:28:50Z

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

            // In that case we still commit into the next local generation.
            if (incomingGeneration != this.lastReceivedPrimaryGen) {
-                flush(false, true);
+                if (engineConfig.getIndexSettings().isStoreLocalityPartial() == false) {


this engine will only flush here and on close. I don't think we have a reason to commit on close as we will sync from store on re-open? If thats the case lets push this logic to the flush method call and cover both cases? Please also add a comment to why we are not committing here.

mch2 · 2024-07-09T21:31:05Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

                for (String file : uploadedSegments.keySet()) {
                    long checksum = Long.parseLong(uploadedSegments.get(file).getChecksum());
-                    if (overrideLocal || localDirectoryContains(storeDirectory, file, checksum) == false) {
+                    if (shouldOverrideLocalFiles || localDirectoryContains(storeDirectory, file, checksum) == false) {


Rather than adding all these checks to this method on locality can we create a separate trimmed down sync method for this case?

mch2 · 2024-07-09T21:39:48Z

server/src/main/java/org/opensearch/indices/replication/RemoteStoreReplicationSource.java

                for (StoreFileMetadata fileMetadata : filesToFetch) {
                    String file = fileMetadata.name();
-                    assert directoryFiles.contains(file) == false : "Local store already contains the file " + file;
+                    assert directoryFiles.contains(file) == false || indexShard.indexSettings().isStoreLocalityPartial()


Can we not skip the call to getSegmentFiles entirely? That would greatly reduce the changes to SegmentReplicationTarget.

mch2 · 2024-07-09T21:45:01Z

server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java

        cancellableThreads.checkForCancel();
        state.setStage(SegmentReplicationState.Stage.FILE_DIFF);
-        final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), indexShard.getSegmentMetadataMap());
+        final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), finalReplicaMd);


I don't think we need to compute a diff here if all files are remote and simply return early if data locality is partial. The Checkpoint info call will include the sis bytes which we load onto the readermanager.

mch2 · 2024-07-09T21:48:30Z

...er/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java

-            verifyStoreContent();
+            // skipping verify store content over here as readLastCommittedSegmentsInfo files are not present in latest metadata of remote
+            // store.
+            if (!warmIndexSegmentReplicationEnabled()) {


i know similar logic was implemented with remote store but I would prefer we not couple these test classes together and instead write new tests where appropriate for warm cases. Or, we break up the SegmentReplicationIT such that reusable tests move to the base IT.

mch2 · 2024-07-09T21:48:37Z

...erTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java

+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)
+@ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class)
+public class WarmIndexRemoteStoreSegmentReplicationIT extends SegmentReplicationIT {


there are likely tests in the regular segrep suite that aren't valid here. Particularly when asserting against store content.

ankitkala · 2024-07-09T06:18:57Z

...er/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java

+            // skipping verify store content over here as readLastCommittedSegmentsInfo files are not present in latest metadata of remote
+            // store.
+            if (!warmIndexSegmentReplicationEnabled()) {
+                verifyStoreContent();


Can we add a backlog issue to fix this?

opensearch-trigger-bot · 2024-08-13T15:21:49Z

This PR is stalled because it has been open for 30 days with no activity.

skumawat2025 · 2025-02-28T08:04:44Z

Closing this PR and starting a new PR for this change: #17390

nisgoel-amazon added 2 commits July 8, 2024 15:51

Making changes for replication and recovery flow for writable warm index

1ef59a0

Signed-off-by: Nishant Goel <nisgoel@amazon.com>

Fixing github workflows and clearing up IT file

41fd1ec

Signed-off-by: Nishant Goel <nisgoel@amazon.com>

github-actions bot added enhancement Enhancement or improvement to existing feature or request Storage:Remote labels Jul 8, 2024

Adding PR in Change log file

3004994

Signed-off-by: Nishant Goel <nisgoel@amazon.com>

rayshrey added the backport 2.x Backport to 2.x branch label Jul 8, 2024

Adding PR in Change log file

d42263b

Signed-off-by: Nishant Goel <nisgoel@amazon.com>

nisgoel-amazon marked this pull request as ready for review July 8, 2024 12:49

nisgoel-amazon requested review from Bukhtawar, CEHENKLE, Rishikesh1159, anasalkouz, andrross, ashking94, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners July 8, 2024 12:49

nisgoel-amazon requested a review from VachaShah as a code owner July 8, 2024 12:49

mch2 reviewed Jul 9, 2024

View reviewed changes

rayshrey added the v2.16.0 Issues and PRs related to version 2.16.0 label Jul 10, 2024

ankitkala reviewed Jul 12, 2024

View reviewed changes

neetikasinghal mentioned this pull request Jul 16, 2024

HotToWarmTieringService changes and changes in shard balancer to tier shards neetikasinghal/OpenSearch#144

Draft

6 tasks

prudhvigodithi mentioned this pull request Jul 22, 2024

[RELEASE] Release version 2.16.0 opensearch-project/opensearch-build#4771

Closed

74 tasks

neetikasinghal mentioned this pull request Jul 23, 2024

HotToWarmTieringService changes to tier shards #14891

Closed

3 tasks

opensearch-trigger-bot bot added the stalled Issues that have stalled label Aug 13, 2024

opensearch-ci-bot mentioned this pull request Sep 11, 2024

[AUTOCUT] Gradle Check Flaky Test Report for MinimumClusterManagerNodesIT #14289

Open

skumawat2025 mentioned this pull request Feb 19, 2025

Writable warm replica replication/recovery #17390

Merged

3 tasks

skumawat2025 closed this Feb 28, 2025

github-project-automation bot moved this from Now(This Quarter) to ✅ Done in Storage Project Board Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Writable warm relocation recovery#14670

Writable warm relocation recovery#14670
nisgoel-amazon wants to merge 4 commits intoopensearch-project:mainfrom
rayshrey:writable-warm-relocation-recovery

nisgoel-amazon commented Jul 8, 2024

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

mch2 Jul 9, 2024

Uh oh!

ankitkala Jul 9, 2024

Uh oh!

opensearch-trigger-bot bot commented Aug 13, 2024

Uh oh!

skumawat2025 commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nisgoel-amazon commented Jul 8, 2024

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mch2 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

ankitkala Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Aug 13, 2024

Uh oh!

skumawat2025 commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants