HADOOP-19394. Integrate with AAL's readVectored(). #7720
Merged
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
|
@steveloughran @mukund-thakur could you please review this initial version, couple of small TODO's coming (more tests, and using the |
rajdchak
reviewed
May 29, 2025
| * @throws IOException IOE if any. | ||
| */ | ||
| @Override | ||
| public synchronized void readVectored(List<? extends FileRange> ranges, |
There was a problem hiding this comment.
just for my knowledge- why is this a synchronised method?
There was a problem hiding this comment.
sorry my miss.. we just discussed, this does not need to be synchronized. will cut
rajdchak
reviewed
May 29, 2025
...op-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStream.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
why the SDK upgrade? |
ahmarsuhail
force-pushed
the
aal-read-vectored
branch
from
3219341 to
5121093
Compare
|
💔 -1 overall
This message was automatically generated. |
4 tasks
ahmarsuhail
force-pushed
the
aal-read-vectored
branch
from
5121093 to
142d1bb
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Jul 9, 2025
hadoop-project/pom.xml
Outdated
| <aws-java-sdk-v2.version>2.29.52</aws-java-sdk-v2.version> | ||
| <amazon-s3-encryption-client-java.version>3.1.1</amazon-s3-encryption-client-java.version> | ||
| <amazon-s3-analyticsaccelerator-s3.version>1.0.0</amazon-s3-analyticsaccelerator-s3.version> | ||
| <amazon-s3-analyticsaccelerator-s3.version>1.2.0</amazon-s3-analyticsaccelerator-s3.version> |
There was a problem hiding this comment.
for the final merge, this should be pulled out to its own.
should we try to get into 3.4.2 now?
| import java.util.function.Consumer; | ||
| import java.util.function.IntFunction; | ||
|
|
||
| import org.apache.hadoop.fs.FileRange; |
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} |
There was a problem hiding this comment.
I'd put that at L148 and cut the params/IOE as superfluous
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} |
| return new S3AContract(conf); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
add a javadoc explaining why the override
| public void testReadVectoredWithAALStatsCollection() throws Exception { | ||
|
|
||
| List<FileRange> fileRanges = createSampleNonOverlappingRanges(); | ||
| try (FSDataInputStream in = openVectorFile()){ |
|
|
||
| // AAL does not do any range coalescing, so input and combined ranges are the same. | ||
| this.getS3AStreamStatistics().readVectoredOperationStarted(ranges.size(), ranges.size()); | ||
| inputStream.readVectored(objectRanges, allocate, release); |
There was a problem hiding this comment.
does this call release on errors? curious -and hopeful
There was a problem hiding this comment.
ah no :( release isn't called right now, but will make that change and get it released in the next version
|
|
||
| List<FileRange> fileRanges = createSampleNonOverlappingRanges(); | ||
| try (FSDataInputStream in = openVectorFile()){ | ||
| in.readVectored(fileRanges, getAllocate()); |
There was a problem hiding this comment.
Are we not verifying the data after vectored read.
There was a problem hiding this comment.
yes, we should be. lot easy to do faster vector IO if you always return the same array of emtpy bytes
There was a problem hiding this comment.
added in validation
...test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractAnalyticsStreamVectoredRead.java
Show resolved
Hide resolved
ahmarsuhail
force-pushed
the
aal-read-vectored
branch
from
786dac7 to
6f730d5
Compare
|
thanks @mukund-thakur and @steveloughran for the reviews, addressed the feedback. Will need to cut a PR to AAL for the release() changes 1/ call release on failure and 2/ add in null check so that Don't think think we need to block this on the above, i will create a new PR to update once there is a new version of AAL |
1 task
|
Addressed the release() changes in: awslabs/analytics-accelerator-s3#337 |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
force-pushed
the
aal-read-vectored
branch
from
8aa9a1e to
f334768
Compare
|
@steveloughran @mukund-thakur i've address feedback on this, could you please review? |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
approved these changes
Aug 26, 2025
| import org.junit.jupiter.params.ParameterizedClass; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| import java.util.List; |
There was a problem hiding this comment.
nit: prefer the java.* stuff in a block just under line 20.
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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
Adds in support for AAL's readVectored().
How was this patch tested?
Testing in progress.