HADOOP-18378. Implement lazy seek in S3A prefetching. #4955
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make S3APrefetchingInputStream.seek() completely lazy. Calls to seek() will not affect the current buffer nor interfere with prefetching, until read() is called. This change allows various usage patterns to benefit from prefetching, e.g. when calling readFully(position, buffer) in a loop for contiguous positions the intermediate internal calls to seek() will be noops and prefetching will have the same performance as in a sequential read.
steveloughran
requested changes
Sep 30, 2022
There was a problem hiding this comment.
looks OK on a first read, but it is complex and goes near to off-by-one bugs in the past. would suggest review by @mukund-thakur , @mehakmeet and anyone else
...project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/prefetch/TestFilePosition.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/prefetch/S3ACachingInputStream.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3APrefetchingInputStream.java
Outdated
Show resolved
Hide resolved
| assertEquals(0, inputStream.read()); | ||
| assertEquals(bufferSize - 1, inputStream.available()); |
There was a problem hiding this comment.
can you add string of what is being tested. the assertEquals prints the values, but it is important to know what is being validated. same below.
actually, given the #of calls, what about a method assertAvailable(InputStream, int) to do the probe everywhere
| @@ -116,7 +116,7 @@ public void setData(BufferData bufferData, | |||
| readOffset, | |||
| "readOffset", | |||
| startOffset, | |||
| startOffset + bufferData.getBuffer().limit() - 1); | |||
| startOffset + bufferData.getBuffer().limit()); | |||
There was a problem hiding this comment.
going to trust you here. we have shipped the s3a input stream with an off by one error in the past. surfaced in parquet but not ORC, and only in some cases...
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Oct 4, 2022
| private static void assertAvailable(int expected, InputStream inputStream) | ||
| throws IOException { | ||
| assertThat(inputStream.available()) | ||
| .describedAs("Check available bytes") |
There was a problem hiding this comment.
in description, include inputStream string value, as it should be reporting its iostats
There was a problem hiding this comment.
Unfortunately, S3ARemoteInputStream.toString() only reports things like nextReadPos and fpos (no iostats!). Still useful, so I've included it anyway. But happy to also review/improve toString() if required, here or on a separate PR.
Note that:
- this PR already modifies
toString(), but only to take the lazy seek logic into account (and make it consistent acrossS3ARemoteInputStreamand its sub-classes) S3AInputStreamalso does not seem to report iostats (unless I'm missing something)
There was a problem hiding this comment.
S3AInputStream.toString():
String s = streamStatistics.toString();
the cloudstore bandwidth command does the toString calls if you ask for a -verbose run, which lets it print stats while still compiling against older releases
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
approved these changes
Oct 6, 2022
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
Make S3APrefetchingInputStream.seek() completely lazy. Calls to seek() will not affect the current buffer nor interfere with prefetching, until read() is called. This change allows various usage patterns to benefit from prefetching, e.g. when calling readFully(position, buffer) in a loop for contiguous positions the intermediate internal calls to seek() will be noops and prefetching will have the same performance as in a sequential read. Contributed by Alessandro Passaro.
4 tasks
asfgit
pushed a commit
that referenced
this pull request
Apr 26, 2023
Make S3APrefetchingInputStream.seek() completely lazy. Calls to seek() will not affect the current buffer nor interfere with prefetching, until read() is called. This change allows various usage patterns to benefit from prefetching, e.g. when calling readFully(position, buffer) in a loop for contiguous positions the intermediate internal calls to seek() will be noops and prefetching will have the same performance as in a sequential read. Contributed by Alessandro Passaro.
asfgit
pushed a commit
that referenced
this pull request
Apr 28, 2023
Make S3APrefetchingInputStream.seek() completely lazy. Calls to seek() will not affect the current buffer nor interfere with prefetching, until read() is called. This change allows various usage patterns to benefit from prefetching, e.g. when calling readFully(position, buffer) in a loop for contiguous positions the intermediate internal calls to seek() will be noops and prefetching will have the same performance as in a sequential read. Contributed by Alessandro Passaro.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
HADOOP-18378. Make S3APrefetchingInputStream.seek() completely lazy. Calls to seek() will not affect the current buffer nor interfere with prefetching, until read() is called. This change allows various usage patterns to benefit from prefetching, e.g. when calling readFully(position, buffer) in a loop for contiguous positions, the intermediate internal calls to seek() will be noops and prefetching will have the same performance as in a sequential read.
How was this patch tested?
Tested with
mvn clean verifyon a bucket ineu-west-2.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?