Enable markSupported in SlicedInputStream #112563

kingherc · 2024-09-05T15:37:29Z

This means openSlice must be able to be called for a previous slice even if the previous slice has been closed.

We disable markSupported in older implementation to keep the same behavior for them.

Relates ES-9248

This means openSlice must be able to be called for a previous slice even if the previous slice has been closed. This is possible for current implementations as a slice corresponds to a blob from the object store, which can be re-opened. Relates ES-9248

elasticsearchmachine · 2024-09-05T19:12:41Z

Pinging @elastic/es-distributed (Team:Distributed)

ywangd

Besides a few minor comments below, I also wonder whether you have considered adding the changes as a subclass in the stateless codebase? Not that I find any change here is unsafe, but a tighter scope generally feels safer to me.

ywangd · 2024-09-06T05:16:27Z

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

+        // We ignore readLimit since openSlice() can re-open previous InputStreams, and we can skip as many bytes as we'd like.
+        // According to JDK documentation, marking a closed InputStream should have no effect.
+        if (closed == false) {
+            if (initialized && nextSlice > 0) { // nextSlice > 0 guards the case it is initialized and numSlices is 0.


How it possible for initialized = true and nextSlice = 0 unless there are concurrent invocation on nextStream and mark? But this class seems to assume single threaded usage?

Sorry, maybe my comment misled you to think of concurrency.

There's indeed a single-threaded edge case where it can happen: if you create a SlicedInputStream with 0 slices (effectively an empty InputStream), and try to read, then the SlicedInputStream is initialized and nextSlice = 0. That's why I had added the nextSlice>0 guard here.

However, I reworked the code to make it better: I added an explicit numSlices > 0 guard to both mark/reset to make them ineffective. And added a test testMarkResetZeroSlices that covers this edge case.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

kingherc

Thanks @ywangd for the review! I handled your feedback, so feel free to review again.

Besides a few minor comments below, I also wonder whether you have considered adding the changes as a subclass in the stateless codebase? Not that I find any change here is unsafe, but a tighter scope generally feels safer to me.

Several reasons: (a) I believe the mark/reset functionality may be useful in the main codebase, (b) the mark/reset code is more readable and easier to do in the same class rather than in a seperate sub-class, (c) some functions are final and did not want to tamper with them (I only removed final from the close function because we need to override it in stateless).

However, I agree the tighter scope would be safer, so I made the current implementations to override markSupported() to false, and it is true only for the stateless implementation.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

kingherc · 2024-09-06T09:27:41Z

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

+        // We ignore readLimit since openSlice() can re-open previous InputStreams, and we can skip as many bytes as we'd like.
+        // According to JDK documentation, marking a closed InputStream should have no effect.
+        if (closed == false) {
+            if (initialized && nextSlice > 0) { // nextSlice > 0 guards the case it is initialized and numSlices is 0.


Sorry, maybe my comment misled you to think of concurrency.

There's indeed a single-threaded edge case where it can happen: if you create a SlicedInputStream with 0 slices (effectively an empty InputStream), and try to read, then the SlicedInputStream is initialized and nextSlice = 0. That's why I had added the nextSlice>0 guard here.

However, I reworked the code to make it better: I added an explicit numSlices > 0 guard to both mark/reset to make them ineffective. And added a test testMarkResetZeroSlices that covers this edge case.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

…-stream

henningandersen

LGTM.

henningandersen · 2024-09-06T12:08:32Z

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

+    public void reset() throws IOException {
+        if (markSupported()) {
+            // JDK documentation does not clarify if a closed InputStream (that has been previously marked) can be reset. We assume the same
+            // behavior specified for mark(), i.e., that reset on a closed InputStream has no effect.


I'd prefer to fail on a closed stream here. Even assert false too.

I made it throw if closed. Did not add an assertion, so I can test the behavior in the unit test. Feel free to tell me if the assertion is important to add.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

henningandersen · 2024-09-06T12:14:35Z

server/src/main/java/org/elasticsearch/indices/recovery/SnapshotFilesProvider.java

+                @Override
+                public boolean markSupported() {
+                    return false;
+                }


I am not sure this is important? Can we remove it? Similar for the next one.

There's a question whether we'd like current implementations to have markSupported as true? I originally enabled them in the PR because I believe they are resumable as they read blobs. But @ywangd mentioned:

I wonder whether you have considered adding the changes as a subclass in the stateless codebase? Not that I find any change here is unsafe, but a tighter scope generally feels safer to me.

In light of this, I disabled markSupported in current implementations. They will behave as they were (without resumability). But feel free to tell me if I should go ahead and enable markSupported in all current implementations.

I was thinking to add a separate subclass for SlicedInputStream and implement mark/reset support in the subclass so that existing usages of SlicedInputStream keep unchanged. But I guess it might be challenging to implement the new features in a separate class? The current version works as well. I slightly prefer to have a follow-up PR just to enable the mark/reset support for existing usages since this PR is to support the stateless side changes. Separating them seems to make that concept clearer.

Indeed it'd be challenging to have them separate, plus I believe it's useful to have the capability in the core SlicedInputStream since all implementations seem to support mark/reset (even though we do not enable them just to maintain their old behavior just to be safe).

server/src/test/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStreamTests.java

kingherc

Thanks for the feedback! I handled it. Added an isClosed() function as well which is needed in the stateless PR. I am in the habit of resolving conversations I think I handled, but feel free to re-open them.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

kingherc · 2024-09-06T13:17:24Z

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

+    public void reset() throws IOException {
+        if (markSupported()) {
+            // JDK documentation does not clarify if a closed InputStream (that has been previously marked) can be reset. We assume the same
+            // behavior specified for mark(), i.e., that reset on a closed InputStream has no effect.


I made it throw if closed. Did not add an assertion, so I can test the behavior in the unit test. Feel free to tell me if the assertion is important to add.

kingherc · 2024-09-06T13:22:13Z

server/src/main/java/org/elasticsearch/indices/recovery/SnapshotFilesProvider.java

+                @Override
+                public boolean markSupported() {
+                    return false;
+                }


There's a question whether we'd like current implementations to have markSupported as true? I originally enabled them in the PR because I believe they are resumable as they read blobs. But @ywangd mentioned:

I wonder whether you have considered adding the changes as a subclass in the stateless codebase? Not that I find any change here is unsafe, but a tighter scope generally feels safer to me.

In light of this, I disabled markSupported in current implementations. They will behave as they were (without resumability). But feel free to tell me if I should go ahead and enable markSupported in all current implementations.

ywangd

LGTM

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

kingherc

Thanks for the reviews! Will merge this PR as soon as it passes the CI.

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java

kingherc · 2024-09-11T08:33:37Z

server/src/main/java/org/elasticsearch/indices/recovery/SnapshotFilesProvider.java

+                @Override
+                public boolean markSupported() {
+                    return false;
+                }


Indeed it'd be challenging to have them separate, plus I believe it's useful to have the capability in the core SlicedInputStream since all implementations seem to support mark/reset (even though we do not enable them just to maintain their old behavior just to be safe).

This means openSlice must be able to be called for a previous slice even if the previous slice has been closed. We disable the support for current implementations to keep their old behavior just to be sure. Relates ES-9248

kingherc added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Sep 5, 2024

kingherc self-assigned this Sep 5, 2024

elasticsearchmachine added the v8.16.0 label Sep 5, 2024

kingherc added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 5, 2024

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Sep 5, 2024

kingherc marked this pull request as ready for review September 5, 2024 19:12

kingherc requested review from fcofdez and ywangd September 5, 2024 19:12

ywangd reviewed Sep 6, 2024

View reviewed changes

kingherc added 2 commits September 6, 2024 12:07

Optional markSupported and consider zero slices

1a83a0d

Reset skips bytes only on marked stream

b597b28

kingherc commented Sep 6, 2024

View reviewed changes

kingherc requested a review from ywangd September 6, 2024 09:42

kingherc added 2 commits September 6, 2024 12:50

Merge remote-tracking branch 'main' into non-issue/adapt-sliced-input…

093a18d

…-stream

Merge remote-tracking branch 'main' into non-issue/adapt-sliced-input…

b966546

…-stream

kingherc force-pushed the non-issue/adapt-sliced-input-stream branch from b966546 to 093a18d Compare September 6, 2024 10:08

henningandersen approved these changes Sep 6, 2024

View reviewed changes

kingherc added 2 commits September 6, 2024 15:35

Add isClosed function

3ba71b5

PR comments

cf7a667

kingherc commented Sep 6, 2024

View reviewed changes

Fix order

c8e0002

kingherc removed the serverless-linked Added by automation, don't add manually label Sep 6, 2024

ywangd approved these changes Sep 9, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStream.java Outdated Show resolved Hide resolved

kingherc added 2 commits September 11, 2024 11:03

Merge commit 'c47162a' into non-issue/adapt-sliced-input-stream

2bfc640

PR comments

9beeacb

kingherc commented Sep 11, 2024

View reviewed changes

kingherc merged commit be90011 into elastic:main Sep 11, 2024
15 checks passed

kingherc deleted the non-issue/adapt-sliced-input-stream branch September 11, 2024 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable markSupported in SlicedInputStream #112563

Enable markSupported in SlicedInputStream #112563

kingherc commented Sep 5, 2024 •

edited

Loading

elasticsearchmachine commented Sep 5, 2024

ywangd left a comment

ywangd Sep 6, 2024

kingherc Sep 6, 2024

kingherc left a comment

kingherc Sep 6, 2024

henningandersen left a comment

henningandersen Sep 6, 2024

kingherc Sep 6, 2024

henningandersen Sep 6, 2024

kingherc Sep 6, 2024

ywangd Sep 9, 2024

kingherc Sep 11, 2024

kingherc left a comment

kingherc Sep 6, 2024

kingherc Sep 6, 2024

ywangd left a comment

kingherc left a comment

kingherc Sep 11, 2024

Enable markSupported in SlicedInputStream #112563

Enable markSupported in SlicedInputStream #112563

Conversation

kingherc commented Sep 5, 2024 • edited Loading

elasticsearchmachine commented Sep 5, 2024

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingherc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingherc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

kingherc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingherc commented Sep 5, 2024 •

edited

Loading