[Remote Segment Store] Add RemoteDirectory interface to copy segment files to/from remote store #3102

sachinpkale · 2022-04-28T11:24:44Z

Description

As described in the #2700, all the read/write operations to remote segment store will happen via RemoteDirectory interface. This change adds the RemoteDirectory interface along with corresponding RemoteIndexInput (extends IndexInput) and RemoteIndexOutput (extends IndexOutput) classes.

Issues Resolved

[Remote Segment Store] Define RemoteDirectory interface #2993

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

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.

opensearch-ci-bot · 2022-04-28T12:00:35Z

❌ Gradle Check failure fa3a260fcd8fe5c27a8227545fe743a3339b4210
Log 4852

Reports 4852

opensearch-ci-bot · 2022-04-28T12:00:51Z

❌ Gradle Check failure 9a6812d9447c580d2b11deda20db9a09614ad5d9
Log 4853

Reports 4853

sachinpkale · 2022-04-28T13:15:16Z

Checking the failures.

opensearch-ci-bot · 2022-04-28T13:40:50Z

❌ Gradle Check failure 91ebb0f57a39d402ef5957b61496503744e1e36a
Log 4856

Reports 4856

Bukhtawar

Thanks Sachin, this is a good start, few minor sugestions/clarifications

server/src/main/java/org/opensearch/index/store/RemoteDirectory.java

Bukhtawar · 2022-05-02T09:08:56Z

server/src/main/java/org/opensearch/index/store/RemoteDirectory.java

+    @Override
+    public Lock obtainLock(String name) throws IOException {
+        throw new UnsupportedOperationException();
+    }
+}


Do we need to acquire a lock before segment uploads or can we support concurrent uploads as well

As long as segments in the remote store are uploaded and downloaded for backup and restore part, I don't think locking is required. Once we start writing to remote store via IndexWriter, locking would be required.

Open to suggestions.

If we extend BaseDirectory, it will provide locking functionality with the provided LockFactory.

Bukhtawar · 2022-05-02T10:03:07Z

server/src/main/java/org/opensearch/index/store/RemoteIndexOutput.java

+    @Override
+    public void copyBytes(DataInput input, long numBytes) throws IOException {
+        assert input instanceof IndexInput : "input should be instance of IndexInput";
+        blobContainer.writeBlob(getName(), new InputStreamIndexInput((IndexInput) input, numBytes), numBytes, false);
+    }
+


Lets say if we go with concurrent writes to local and remote store would we still need a copyBytes or writeByte

Do we also use ByteBuffer for leveraging I/O optimizations?

Lets say if we go with concurrent writes to local and remote store would we still need a copyBytes or writeByte

For concurrent writes, we will need writeByte/s

Do we also use ByteBuffer for leveraging I/O optimizations?

Right. Current implementation may not be optimized. It also depends on how a given repository implementation is handling writeBlob.
We will be having a continuous performance test setup which will provide insight into the performance issues.

Another case, let's say it's not concurrent, but we want writes to only happen on remote which can be directly searchable we might want to just do writeBytes to remote directly

Right, the idea is to open up the methods as per the requirement. As you said, writeByte/writeBytes methods will be required when IndexWriter is using RemoteDirectory to create the segments (concurrent writes or writing only to remote store).

Bukhtawar · 2022-05-02T10:05:42Z

server/src/main/java/org/opensearch/index/store/RemoteIndexInput.java

+public class RemoteIndexInput extends IndexInput {
+
+    private final InputStream inputStream;
+    private final long size;
+
+    public RemoteIndexInput(String name, InputStream inputStream, long size) {
+        super(name);
+        this.inputStream = inputStream;
+        this.size = size;
+    }
+
+    @Override


We also have a VerifyingIndexInput we might want to consider that handles the checksums

Yes, either VerifyingIndexInput or ChecksumIndexInput can be used. I have added it as a ToDo item in the javadoc of the class.

Issue is created for Checksum. Tagging this comment on the issue.

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

opensearch-ci-bot · 2022-05-03T12:03:55Z

❌ Gradle Check failure d11d190
Log 4925

Reports 4925

sachinpkale · 2022-05-04T12:12:01Z

@dblock @nknize Please review.

dblock · 2022-05-06T14:42:36Z

You have failing tests here, do you still want to merge? (feature branch)

sachinpkale · 2022-05-06T16:20:26Z

You have failing tests here, do you still want to merge? (feature branch)

This seem to be like a flaky test. There was a fix provided for it as a part of #2110. As this feature-branch was created before the fix, test is failing. Changes in this PR should not impact any existing tests.
Once this PR is merged, I will get main branch changes merged to this feature branch to keep it updated.

dblock · 2022-05-06T16:30:13Z

start gradle check

opensearch-ci-bot · 2022-05-06T17:02:12Z

❌ Gradle Check failure d11d190
Log 5091

Reports 5091

Bukhtawar · 2022-05-06T20:10:35Z

start gradle check

Bukhtawar · 2022-05-06T20:11:44Z

@sachinpkale @dblock I have rebased the feature branch from main and tried triggering a fresh build

owaiskazi19 · 2022-05-06T20:37:41Z

start gradle check

dblock · 2022-05-06T20:51:11Z

start gradle check

peterzhuamazon · 2022-05-06T21:32:40Z

start gradle check

peterzhuamazon · 2022-05-06T21:36:53Z

start gradle check try one more time.

opensearch-ci-bot · 2022-05-06T22:26:46Z

❌ Gradle Check failure d11d190
Log 5108

Reports 5108

sachinpkale · 2022-05-07T01:12:46Z

Checked the logs of last run. The failing test is: :rest-api-spec:yamlRestTest. This is a flaky test, more details at: #1817

Bukhtawar · 2022-05-07T06:16:00Z

start gradle check

opensearch-ci-bot · 2022-05-07T06:50:22Z

✅ Gradle Check success d11d190
Log 5124

Reports 5124

peterzhuamazon · 2022-05-07T16:21:25Z

Does this needs to be backported to 2.0/2.x or is this purely a 3.0 change?

dblock · 2022-05-10T15:31:33Z

@sachinpkale @Bukhtawar ^

sachinpkale · 2022-05-10T15:57:43Z

Does this needs to be backported to 2.0/2.x or is this purely a 3.0 change?

@peterzhuamazon This would be a 3.0 change.

…ore (opensearch-project#3102) Signed-off-by: Sachin Kale <kalsac@amazon.com> Co-authored-by: Sachin Kale <kalsac@amazon.com>

sachinpkale requested a review from a team as a code owner April 28, 2022 11:24

sachinpkale force-pushed the feature/durability-enhancements branch from fa3a260 to 9a6812d Compare April 28, 2022 11:33

sachinpkale force-pushed the feature/durability-enhancements branch from 9a6812d to 91ebb0f Compare April 28, 2022 13:18

sachinpkale mentioned this pull request Apr 28, 2022

[Remote Segment Store] Define RemoteDirectory interface #2993

Closed

Bukhtawar reviewed May 2, 2022

View reviewed changes

sachinpkale mentioned this pull request May 2, 2022

[Searchable Snapshot] Design and define the interfaces for file and directory structures for Remote Storage #3114

Closed

Add RemoteDirectory interface to copy segment files to/from remote store

d11d190

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

sachinpkale force-pushed the feature/durability-enhancements branch from 91ebb0f to d11d190 Compare May 3, 2022 11:37

sachinpkale mentioned this pull request May 3, 2022

[Remote Segment Store] Validate checksum of uploaded and downloaded segment files #3143

Open

Bukhtawar approved these changes May 6, 2022

View reviewed changes

Bukhtawar assigned sachinpkale May 6, 2022

Bukhtawar added the Storage:Durability Issues and PRs related to the durability framework label May 6, 2022

Bukhtawar merged commit 0f587d2 into opensearch-project:feature/durability-enhancements May 7, 2022

sachinpkale mentioned this pull request May 27, 2022

[Remote Store] Upload segments to remote store post refresh #3460

Merged

5 tasks

[Remote Segment Store] Add RemoteDirectory interface to copy segment files to/from remote store #3102

[Remote Segment Store] Add RemoteDirectory interface to copy segment files to/from remote store #3102

Conversation

sachinpkale commented Apr 28, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Apr 28, 2022

opensearch-ci-bot commented Apr 28, 2022

sachinpkale commented Apr 28, 2022

opensearch-ci-bot commented Apr 28, 2022

Bukhtawar left a 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 May 2, 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

opensearch-ci-bot commented May 3, 2022

sachinpkale commented May 4, 2022

dblock commented May 6, 2022

sachinpkale commented May 6, 2022

dblock commented May 6, 2022

opensearch-ci-bot commented May 6, 2022

Bukhtawar commented May 6, 2022

Bukhtawar commented May 6, 2022

owaiskazi19 commented May 6, 2022

dblock commented May 6, 2022

peterzhuamazon commented May 6, 2022

peterzhuamazon commented May 6, 2022

opensearch-ci-bot commented May 6, 2022

sachinpkale commented May 7, 2022

Bukhtawar commented May 7, 2022

opensearch-ci-bot commented May 7, 2022

peterzhuamazon commented May 7, 2022

dblock commented May 10, 2022

sachinpkale commented May 10, 2022

sachinpkale commented Apr 28, 2022 •

edited

Loading

Bukhtawar May 2, 2022 •

edited

Loading