[Remote Store] Add support for refresh level durability #5253

sachinpkale · 2022-11-15T08:26:41Z

Description

As part of remote store feature that was introduced in OpenSearch 2.3, segments are uploaded to remote store post each refresh.
This means remote segment store contains segments that were part of last refresh.
But the _remotestore/_restore API only supports commit level durability.
This feature will add refresh level durability support to the restore API.

Proposed Solution

We upload SegmentInfosSnapshot during each refresh and keep the information of the uploaded file in the metadata file.
During restore, if SegmentInfosSnapshot is present, we use it as a SegmentInfos file and trigger a commit.
As the snapshot file does not contain latest processed checkpoint (as it is a snapshot of previous SegmentInfos file), we pass processed checkpoint as part of filename.

Issues Resolved

[Remote Store] Support refresh level durability #5251

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2022-11-15T08:46:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/6889/
CommitID: 3bdc3d5
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-15T09:36:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/6892/
CommitID: 7a287bc
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

gbbafna · 2022-11-15T10:00:12Z

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

+                            try {
+                                if (segment_info_snapshot_filename != null) {
+                                    storeDirectory.deleteFile(segment_info_snapshot_filename);
+                                }
+                            } catch (IOException e) {
+                                logger.warn("Exception while deleting: " + segment_info_snapshot_filename, e);
+                            }


\why are we deleting segment_info_snapshot_filename ?

Currently, RemoteDirectory does not support writing to file in remote store using buffer (we have a plan to support it). So, for any file to upload to remote store, today, it is required to be in the local directory (Referred as storeDirectory in this code).
There are 2 reasons to delete:

local directory does not support overriding a file with the same name. This is mostly done to keep the immutable nature of segment files.

As this file is of no use locally, we are deleting it once if it is uploaded. Even if the upload fails, next refresh can create a new snapshot file.

github-actions · 2022-11-16T06:57:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
FAILURES:

URL: https://build.ci.opensearch.org/job/gradle-check/6950/
CommitID: cd6bf70
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-16T09:13:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
FAILURES:

URL: https://build.ci.opensearch.org/job/gradle-check/6954/
CommitID: f7d9b2e
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-16T12:21:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/6958/
CommitID: 057a5f3

codecov-commenter · 2022-11-16T12:23:36Z

Codecov Report

Merging #5253 (9562461) into main (cb26035) will increase coverage by 0.04%.
The diff coverage is 91.07%.

@@             Coverage Diff              @@
##               main    #5253      +/-   ##
============================================
+ Coverage     70.95%   71.00%   +0.04%     
- Complexity    58304    58326      +22     
============================================
  Files          4733     4733              
  Lines        278256   278294      +38     
  Branches      40249    40254       +5     
============================================
+ Hits         197441   197594     +153     
+ Misses        64628    64553      -75     
+ Partials      16187    16147      -40

Impacted Files	Coverage Δ
...va/org/opensearch/index/engine/InternalEngine.java	`74.68% <ø> (-1.03%)`	⬇️
...search/index/shard/RemoteStoreRefreshListener.java	`79.24% <88.23%> (+0.93%)`	⬆️
...java/org/opensearch/index/shard/StoreRecovery.java	`68.04% <90.90%> (+0.15%)`	⬆️
...earch/index/store/RemoteSegmentStoreDirectory.java	`99.30% <100.00%> (+0.01%)`	⬆️
...a/org/opensearch/client/cluster/ProxyModeInfo.java	`0.00% <0.00%> (-55.00%)`	⬇️
.../java/org/opensearch/node/NodeClosedException.java	`50.00% <0.00%> (-50.00%)`	⬇️
.../index/shard/IndexShardNotRecoveringException.java	`0.00% <0.00%> (-50.00%)`	⬇️
...ain/java/org/opensearch/search/sort/MinAndMax.java	`63.15% <0.00%> (-36.85%)`	⬇️
...readonly/TransportVerifyShardIndexBlockAction.java	`51.21% <0.00%> (-31.71%)`	⬇️
...opensearch/snapshots/SnapshotMissingException.java	`28.57% <0.00%> (-28.58%)`	⬇️
... and 480 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-11-17T04:05:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
FAILURES:

URL: https://build.ci.opensearch.org/job/gradle-check/6985/
CommitID: 709fd74
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-17T04:05:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
FAILURES:

URL: https://build.ci.opensearch.org/job/gradle-check/6986/
CommitID: 37be057
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dblock · 2022-11-17T17:46:24Z

Rebase pls, the above should be resolved.

github-actions · 2022-11-18T04:10:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/7038/
CommitID: 70a03a7

gbbafna · 2022-11-18T13:23:57Z

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

@@ -44,6 +47,7 @@ public final class RemoteStoreRefreshListener implements ReferenceManager.Refres
    static final Set<String> EXCLUDE_FILES = Set.of("write.lock");
    // Visible for testing
    static final int LAST_N_METADATA_FILES_TO_KEEP = 10;
+    public static final String SEGMENT_INFO_SNAPSHOT_FILENAME = "segment_infos_snapshot_filename";


nit : should we rename to SEGMENT_INFO_SNAPSHOT_FILENAME_PREFIX ?

and please make remove public

gbbafna · 2022-11-18T13:27:38Z

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

            for (String file : remoteDirectory.listAll()) {
                storeDirectory.copyFrom(remoteDirectory, file, file, IOContext.DEFAULT);
+                if (file.startsWith(SEGMENT_INFO_SNAPSHOT_FILENAME)) {
+                    segmentInfosSnapshotFilename = file;


would we need the latest file here. The list wouldn't guarantee sorted in the order we need, right ?

When we create instance of RemoteSegmentStoreDirectory, we initialize it by reading latest metadata file from the remote store. Each metadata file holds reference to only one SegmentInfosSnapshot file.

gbbafna · 2022-11-18T13:38:27Z

server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java

@@ -317,6 +317,33 @@ public void testCopyFromException() throws IOException {
        storeDirectory.close();
    }

+    public void testCpoyFromOverride() throws IOException {


nit : spelling of copy.

Bukhtawar · 2022-11-22T12:39:13Z

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

+        IndexOutput indexOutput = storeDirectory.createOutput(segment_info_snapshot_filename, IOContext.DEFAULT);
+        segmentInfosSnapshot.write(indexOutput);
+        indexOutput.close();


Can we do try with resource here and ensure we close all streams

Bukhtawar · 2022-11-22T13:09:14Z

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

+                    store.commitSegmentInfos(infos_snapshot, Long.parseLong(filenameTokens[2]), Long.parseLong(filenameTokens[2]));
+                } catch (IOException e) {
+                    logger.info("Exception while reading {}, falling back to commit level restore", segmentInfosSnapshotFilename);
+                }
+            }


Seems that the handling isn't correct, f.e if we support refresh level durability but are unable to guarantee it should be not be a silent(info logging) failure. We should instead throw this exception and let the shard decide on the recovery completion

Also lets add assertions on checkpoints and max seq no

@Bukhtawar I will add the required assertions but wanted to check on the first comment.

In the first comment, are you suggesting to fail the restore if refresh level durability is not possible? In that case, we need to provide a way to restore from the last commit if user wants.
Would having a query parameter to the restore API work?

reta · 2022-11-23T14:43:03Z

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

                            deleteStaleCommits();
                        }
+                        String segment_info_snapshot_filename = null;


Please follow the naming conventions

reta · 2022-11-23T14:53:56Z

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

@@ -106,13 +113,19 @@ public void afterRefresh(boolean didRefresh) {
                                .max(Comparator.comparingLong(IndexFileNames::parseGeneration));


Could we use:

final String latestSegmentInfos = segmentInfos.getSegmentsFileName();

Instead of filtering the collection?

segmentInfosFiles can contain multiple segments_N files. We are making sure to get list of all such files and pick only the latest one by generation.

reta · 2022-11-23T15:07:08Z

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

                                refreshedLocalFiles.addAll(SegmentInfos.readCommit(storeDirectory, latestSegmentInfos.get()).files(true));
                                segmentInfosFiles.stream()
                                    .filter(file -> !file.equals(latestSegmentInfos.get()))
                                    .forEach(refreshedLocalFiles::remove);

                                boolean uploadStatus = uploadNewSegments(refreshedLocalFiles);
                                if (uploadStatus) {
+                                    if (segmentFilesFromSnapshot.equals(new HashSet<>(refreshedLocalFiles))) {
+                                        segment_info_snapshot_filename = uploadSegmentInfosSnapshot(latestSegmentInfos.get(), segmentInfos);
+                                        refreshedLocalFiles.add(segment_info_snapshot_filename);


The uploadSegmentInfosSnapshot seems to be uploading the file to remoteDirectory, why do we need to add it to refreshedLocalFiles (and apparently re-upload again with remoteDirectory.uploadMetadata)?

remoteDirectory.uploadMetadata() uploads only the metadata file which keeps the mapping of files uploaded to remote store.

Filename is added to refreshedLocalFiles so that metadata file knows about snapshot file uploaded to remote store.

reta · 2022-11-23T15:07:36Z

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

+    String uploadSegmentInfosSnapshot(String latestSegmentsNFilename, SegmentInfos segmentInfosSnapshot) throws IOException {
+        long localCheckpoint = indexShard.getEngine().getProcessedLocalCheckpoint();
+        String commitGeneration = latestSegmentsNFilename.substring((IndexFileNames.SEGMENTS + "_").length());
+        String segment_info_snapshot_filename = SEGMENT_INFO_SNAPSHOT_FILENAME + "__" + commitGeneration + "__" + localCheckpoint;


Please follow the naming conventions

reta · 2022-11-23T15:10:41Z

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

@@ -141,6 +162,18 @@ public void afterRefresh(boolean didRefresh) {
        }
    }

+    String uploadSegmentInfosSnapshot(String latestSegmentsNFilename, SegmentInfos segmentInfosSnapshot) throws IOException {
+        long localCheckpoint = indexShard.getEngine().getProcessedLocalCheckpoint();
+        String commitGeneration = latestSegmentsNFilename.substring((IndexFileNames.SEGMENTS + "_").length());


IndexFileNames.parseGeneration(latestSegmentsNFilename)?

Used SegmentInfos.generationFromSegmentsFileName instead. IndexFileNames.parseGeneration only works for segment files where name starts with _

github-actions · 2022-11-29T06:23:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7402/
CommitID: 58a125f
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 · 2022-11-29T09:55:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7404/
CommitID: 47db802
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 · 2022-11-29T11:36:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.action.admin.indices.create.ShrinkIndexIT.testShrinkIndexPrimaryTerm
      1 org.opensearch.action.admin.indices.create.ShrinkIndexIT.testCreateShrinkIndexToN
      1 org.opensearch.action.admin.indices.create.ShrinkIndexIT.testCreateShrinkIndexFails
      1 org.opensearch.action.admin.indices.create.ShrinkIndexIT.testCreateShrinkIndex

URL: https://build.ci.opensearch.org/job/gradle-check/7406/
CommitID: 28a161a
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2022-12-15T08:01:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8134/
CommitID: 9562461

sachinpkale · 2022-12-15T08:04:39Z

Can we write Integ tests for this ?

Yes @gbbafna . Currently, we don't have any integ tests written for remote store feature. Will add.

Bukhtawar

Thanks LGTM

gbbafna reviewed Nov 16, 2022

View reviewed changes

sachinpkale marked this pull request as ready for review November 17, 2022 03:56

sachinpkale requested review from a team and reta as code owners November 17, 2022 03:56

sachinpkale force-pushed the feature/refresh-level-durability branch from 37be057 to 70a03a7 Compare November 18, 2022 03:41

gbbafna reviewed Nov 18, 2022

View reviewed changes

Bukhtawar reviewed Nov 22, 2022

View reviewed changes

reta reviewed Nov 23, 2022

View reviewed changes

sachinpkale force-pushed the feature/refresh-level-durability branch from 70a03a7 to 58a125f Compare November 29, 2022 05:58

sachinpkale requested review from reta and gbbafna and removed request for gbbafna November 30, 2022 05:26

Sachin Kale added 2 commits December 15, 2022 12:58

Remove silent handling of IOException in reading SegmentInfosSnapshot

499a425

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Use lastRefreshedCheckpoint insteand of processedCheckpoint

9562461

Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale force-pushed the feature/refresh-level-durability branch from 4547ac4 to 9562461 Compare December 15, 2022 07:28

sachinpkale requested review from anasalkouz, andrross, CEHENKLE, dblock, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners December 15, 2022 07:28

This was referenced Dec 15, 2022

[Remote Store] Segment Replication checkpoint publication to sync with remote store upload #5578

Closed

[Remote Store] Download segments from remote segment store in failover flow #5579

Merged

Bukhtawar approved these changes Dec 16, 2022

View reviewed changes

Bukhtawar merged commit d27c30a into opensearch-project:main Dec 16, 2022

Bukhtawar mentioned this pull request Dec 31, 2022

Adding @gbbafna to OpenSearch maintainers #5668

Merged

2 tasks

sachinpkale mentioned this pull request Jan 9, 2023

[Backport 2.x] Add support for refresh level durability using remote segment store #5749

Merged

6 tasks

		@@ -106,13 +113,19 @@ public void afterRefresh(boolean didRefresh) {
		.max(Comparator.comparingLong(IndexFileNames::parseGeneration));

[Remote Store] Add support for refresh level durability #5253

[Remote Store] Add support for refresh level durability #5253

Conversation

sachinpkale commented Nov 15, 2022 • edited Loading

Description

Proposed Solution

Issues Resolved

Check List

github-actions bot commented Nov 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 15, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 16, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 16, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 16, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Nov 16, 2022 • edited Loading

Codecov Report

github-actions bot commented Nov 17, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 17, 2022

Gradle Check (Jenkins) Run Completed with:

dblock commented Nov 17, 2022

github-actions bot commented Nov 18, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 15, 2022

Gradle Check (Jenkins) Run Completed with:

sachinpkale commented Dec 15, 2022

Bukhtawar left a comment

Choose a reason for hiding this comment

sachinpkale commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 16, 2022 •

edited

Loading

Bukhtawar Nov 22, 2022 •

edited

Loading