Support long marked offset in SlicedInputStream #113629

kingherc · 2024-09-26T16:27:30Z

Else, a reset might reset to an unintended offset in a big slice.

Relates ES-9639

Else, a reset might reset to an unintended offset in a big slice. Relates ES-9639

elasticsearchmachine · 2024-09-26T17:46:23Z

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

DaveCTurner

Fix looks good, I left some suggestions regarding the test.

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

henningandersen

Looks good, left a couple comments in addition to David's.

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

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

…e/ES-9639-fix-sliced-mark-long

…-mark-long

kingherc

Thanks for the feedback @DaveCTurner , @henningandersen ! Handled, feel free to review again.

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

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

DaveCTurner · 2024-09-27T10:37:27Z

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

+                if (currentStream == null) {
+                    // In case EOF has been reached, we set the currentStream to a non-null value so that nextStream() does not complain.
+                    currentStream = InputStream.nullInputStream();
+                }


Do we really need to do this dance just to make nextStream() happy? It seems as if we could just set currentStream to the right slice directly?

Your comment made me thinking and found another non-intrusive way: extend the assertion in nextStream(). Feel free to review!

DaveCTurner · 2024-09-27T10:47:48Z

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

+            }
+            final long skipped = stream.skip(remaining);
+            currentSliceOffset += skipped;
+            if (skipped == 0) {


Hmm skipped == 0 doesn't necessarily mean we reached end-of-stream. If we haven't quite skipped enough bytes we should try and read() one more byte and only move to the next stream if that returns -1.

I think we also shouldn't keep retrying the skip() if it falls short (unless it reached the end of the stream). It's up to the caller to check the return value and retry if needed.

Ah good point! Added a read() to ensure we're reaching stream EOF.

I think we also shouldn't keep retrying the skip() if it falls short (unless it reached the end of the stream). It's up to the caller to check the return value and retry if needed.

I'm split on this. Certainly the caller should retry, but I also see implementations with loops. E.g., the default JDK implementation has a loop over read(), instead of a single call to read(). I also feel it's "safer" in case the caller forgets the loop.

Yeah in practice most impls of skip do try and skip all the way, just seems like if one doesn't then it might be useful to propagate that to the caller.

I don't think we want to check skipped == 0 at all tbh - that involves an extra iteration at the end of each stream. Can we just check for EOF if skipped < remaining?

Yup, totally makes it better, done!

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

kingherc

Thanks @DaveCTurner ! Feel free to review again.

kingherc · 2024-09-27T11:39:54Z

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

+            }
+            final long skipped = stream.skip(remaining);
+            currentSliceOffset += skipped;
+            if (skipped == 0) {


Ah good point! Added a read() to ensure we're reaching stream EOF.

I think we also shouldn't keep retrying the skip() if it falls short (unless it reached the end of the stream). It's up to the caller to check the return value and retry if needed.

I'm split on this. Certainly the caller should retry, but I also see implementations with loops. E.g., the default JDK implementation has a loop over read(), instead of a single call to read(). I also feel it's "safer" in case the caller forgets the loop.

kingherc · 2024-09-27T11:49:23Z

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

+                if (currentStream == null) {
+                    // In case EOF has been reached, we set the currentStream to a non-null value so that nextStream() does not complain.
+                    currentStream = InputStream.nullInputStream();
+                }


Your comment made me thinking and found another non-intrusive way: extend the assertion in nextStream(). Feel free to review!

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

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

DaveCTurner · 2024-09-27T12:02:39Z

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

+            }
+            final long skipped = stream.skip(remaining);
+            currentSliceOffset += skipped;
+            if (skipped == 0) {


Yeah in practice most impls of skip do try and skip all the way, just seems like if one doesn't then it might be useful to propagate that to the caller.

I don't think we want to check skipped == 0 at all tbh - that involves an extra iteration at the end of each stream. Can we just check for EOF if skipped < remaining?

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

kingherc

Thanks @DaveCTurner ! Feel free to review again.

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

kingherc · 2024-09-27T12:09:58Z

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

+            }
+            final long skipped = stream.skip(remaining);
+            currentSliceOffset += skipped;
+            if (skipped == 0) {


Yup, totally makes it better, done!

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

henningandersen

Looks good. Just a few more comments (besides David's).

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

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

kingherc

Thanks for the feedback @henningandersen , @DaveCTurner ! Feel free to review again.

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

DaveCTurner

LGTM (one question but it doesn't especially matter either way)

DaveCTurner · 2024-09-27T13:42:20Z

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

+                    // the skip is performed on the marked slice and no other slices are involved. This may help uncover any bugs.
+                    currentStream.skipNBytes(markedSliceOffset);
+                } else {
+                    currentStream = null;


Can this else branch happen? Isn't markedSlice < numSlices an invariant?

It can happen in the case one marks after reaching EOF. Added a test and made sure it works.

henningandersen

LGTM.

…-mark-long

kingherc · 2024-09-27T14:59:16Z

CI failure is #113694 , retrying.

elasticsearchmachine · 2024-09-27T15:47:46Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Else, a reset might reset to an unintended offset in a big slice. Also add support for skip. Relates ES-9639

Support long marked offset in SlicedInputStream

d49d5f1

Else, a reset might reset to an unintended offset in a big slice. Relates ES-9639

kingherc added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team (obsolete) auto-backport-and-merge v8.16.0 v9.0.0 labels Sep 26, 2024

kingherc self-assigned this Sep 26, 2024

kingherc marked this pull request as ready for review September 26, 2024 17:46

kingherc requested a review from henningandersen September 26, 2024 17:46

DaveCTurner reviewed Sep 26, 2024

View reviewed changes

server/src/test/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStreamTests.java Outdated Show resolved Hide resolved

server/src/test/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStreamTests.java Outdated Show resolved Hide resolved

henningandersen reviewed Sep 27, 2024

View reviewed changes

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

server/src/test/java/org/elasticsearch/index/snapshots/blobstore/SlicedInputStreamTests.java Outdated Show resolved Hide resolved

kingherc added 5 commits September 27, 2024 11:14

Merge commit 'e77dc6dc1324b00ba2ddf15718686e8bd1bcb3a9' into non-issu…

d6373e5

…e/ES-9639-fix-sliced-mark-long

255 bytes and add skip

9f0ac0f

Very big mark and skip in test

d35bb2c

Reset after having reached EOF

ebad79e

Merge remote-tracking branch 'main' into non-issue/ES-9639-fix-sliced…

61b3999

…-mark-long

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

kingherc commented Sep 27, 2024

View reviewed changes

kingherc requested review from DaveCTurner and henningandersen September 27, 2024 10:31

Add random skip to mark test

902ee7e

DaveCTurner reviewed Sep 27, 2024

View reviewed changes

PR comments

0dcb356

kingherc commented Sep 27, 2024

View reviewed changes

kingherc requested a review from DaveCTurner September 27, 2024 11:55

DaveCTurner reviewed Sep 27, 2024

View reviewed changes

PR comments

995feea

kingherc commented Sep 27, 2024

View reviewed changes

kingherc requested a review from DaveCTurner September 27, 2024 12:11

DaveCTurner reviewed Sep 27, 2024

View reviewed changes

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

henningandersen reviewed Sep 27, 2024

View reviewed changes

PR comments

79c53af

kingherc commented Sep 27, 2024

View reviewed changes

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

kingherc requested review from DaveCTurner and henningandersen September 27, 2024 12:34

DaveCTurner approved these changes Sep 27, 2024

View reviewed changes

henningandersen approved these changes Sep 27, 2024

View reviewed changes

kingherc added 2 commits September 27, 2024 16:55

Mark beyond EOF test

3e88630

Merge remote-tracking branch 'main' into non-issue/ES-9639-fix-sliced…

51a185d

…-mark-long

kingherc mentioned this pull request Sep 27, 2024

[CI] DocsClientYamlTestSuiteIT test {yaml=reference/rest-api/usage/line_38} failing #113694

Open

kingherc merged commit dfa13ba into elastic:main Sep 27, 2024
16 checks passed

kingherc mentioned this pull request Sep 27, 2024

[8.x] Support long marked offset in SlicedInputStream (#113629) #113704

Merged

kingherc added a commit to kingherc/elasticsearch that referenced this pull request Sep 27, 2024

Support long marked offset in SlicedInputStream (elastic#113629)

aa69985

Else, a reset might reset to an unintended offset in a big slice. Also add support for skip. Relates ES-9639

elasticsearchmachine pushed a commit that referenced this pull request Sep 27, 2024

Support long marked offset in SlicedInputStream (#113629) (#113704)

6c660a5

Else, a reset might reset to an unintended offset in a big slice. Also add support for skip. Relates ES-9639

matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 4, 2024

Support long marked offset in SlicedInputStream (elastic#113629)

7c34d12

Else, a reset might reset to an unintended offset in a big slice. Also add support for skip. Relates ES-9639

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support long marked offset in SlicedInputStream #113629

Support long marked offset in SlicedInputStream #113629

kingherc commented Sep 26, 2024

elasticsearchmachine commented Sep 26, 2024

DaveCTurner left a comment

henningandersen left a comment

kingherc left a comment

DaveCTurner Sep 27, 2024

kingherc Sep 27, 2024

DaveCTurner Sep 27, 2024

kingherc Sep 27, 2024

DaveCTurner Sep 27, 2024

kingherc Sep 27, 2024

kingherc left a comment

kingherc Sep 27, 2024

kingherc Sep 27, 2024

DaveCTurner Sep 27, 2024

kingherc left a comment

kingherc Sep 27, 2024

henningandersen left a comment

kingherc left a comment

DaveCTurner left a comment

DaveCTurner Sep 27, 2024

kingherc Sep 27, 2024

henningandersen left a comment

kingherc commented Sep 27, 2024

elasticsearchmachine commented Sep 27, 2024

Support long marked offset in SlicedInputStream #113629

Support long marked offset in SlicedInputStream #113629

Conversation

kingherc commented Sep 26, 2024

elasticsearchmachine commented Sep 26, 2024

DaveCTurner left a comment

Choose a reason for hiding this comment

henningandersen 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

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

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

kingherc left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this 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

kingherc commented Sep 27, 2024

elasticsearchmachine commented Sep 27, 2024

💚 Backport successful