HADOOP-11867. Add a high performance vectored read API to file system. #3499

mukund-thakur · 2021-09-29T11:48:01Z

Rebased work on top of #1830

Conflicts:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/BufferedFSInputStream.java
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSDataInputStream.java
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java
pom.xml

Description of PR

Adding support for multiple ranged read async api in PositionedReadable. The default iterates through the ranges to read each synchronously, but the intent is that FSDataInputStream subclasses can make more efficient readers especially object stores implementation.

How was this patch tested?

Added benchmarks.
Added UT's
Added new contract tests for new API spec.

For code changes:

Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

mukund-thakur · 2021-09-29T11:50:01Z

CC @steveloughran @mehakmeet @omalley

steveloughran · 2021-09-29T15:42:20Z

Quick review.

needs filesystem spec
and a contract test
test to include
-overlapping ranges
-beyond file length
-negative values
-on heap and off heap buffers
be good to have a PoC of the s3a one, so that we can happy the API xfers to the store
and some ORC/parquet branch which shows it can be used
is there any API we'd want, e.g an fs command which does a single async ranged get

hadoop fs ranged -start 0 -end 345  -out output.txt s3a://file

would be good for QE tests

and we should have a path capability. Anything which supports seek will also do this (so ftp won't)

hadoop-common-project/benchmark/pom.xml

hadoop-common-project/benchmark/src/main/java/org/apache/hadoop/benchmark/AsyncBenchmark.java

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/PositionedReadable.java

hadoop-common-project/hadoop-common/pom.xml

...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/AsyncReaderUtils.java

...-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/CombinedFileRange.java

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java

bogthe

Did an initial review and looks promising.

I'm super curious to see what an async read will bring in terms of performance improvements for a job (even if it's a quick scrappy implementation in the FS).

hadoop-common-project/benchmark/src/main/java/org/apache/hadoop/benchmark/AsyncBenchmark.java

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java

Conflicts: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/BufferedFSInputStream.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSDataInputStream.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java pom.xml

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/fsdatainputstream.md

steveloughran · 2022-01-06T13:21:13Z

I just realized I had some pending comments on one of the updates; maybe out of date now and they were all minor. Let me re-review.

steveloughran

here's some comments I had outstanding since 2021 .. sorry. will do a bigger review now

...mon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestVectoredReadUtils.java

...-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/VectoredReadUtils.java

steveloughran · 2021-12-03T18:23:28Z

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java

@@ -66,7 +74,7 @@
  public static double getApproxChkSumLength(long size) {
    return ChecksumFSOutputSummer.CHKSUM_AS_FRACTION * size;
  }
-  
+


These are all needless and making the patch a bit bigger than it should be. was yetus complaining?

I don't know how it is showing up here. In intellJ diff I don't see this.

steveloughran · 2022-01-12T17:45:20Z

...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractVectoredReadTest.java

+    FileSystem fs = getFileSystem();
+    List<FileRange> fileRanges = new ArrayList<>();
+    fileRanges.add(new FileRangeImpl(1293, 25837));
+    try (FSDataInputStream in = fs.open(path(VECTORED_READ_FILE_1MB_NAME))) {


can you use openFile here? just to make sure that codepath is happy

...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractVectoredReadTest.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java

steveloughran · 2022-01-12T18:01:12Z

OK, I've had a look at the recent changes, and had some shared screen reviews over zoom

here's what I propose

create a feature branch in the asf repo where we can rebase/reorder commits
get what is done in as the base commit
then we can split work on the final changes up into new PRs, each with their own JIRA.

where those changes can be

buffer pooling (API to these calls and better weak reference handling in the elastic one or nearby)
s3a split handling
huge file support (new tests in the s3a huge file suite)

then, once happy we can do a rebase followed by merge commit into trunk/branch-3.

key point: this patch has been going and is ready to stabilize, which can be done with incremental changes in a feature branch.

mehakmeet

Did an initial Review, looks really good.
I think we should add an integration test around the ordered disjoint list of ranges as well to showcase them not merging and reading or having a few ranges where some are merged and some aren't by validating them.
Also, are both minSeek and maxReadSize going to be configurable? I assume they are hardcoded just so you can test them out..

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java

...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractVectoredReadTest.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java

hadoop-tools/hadoop-aws/src/test/resources/log4j.properties

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MoreAsserts.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java

...mmon/src/test/java/org/apache/hadoop/fs/contract/localfs/TestLocalFSConractVectoredRead.java

mukund-thakur · 2022-01-19T09:37:03Z

Did an initial Review, looks really good. I think we should add an integration test around the ordered disjoint list of ranges as well to showcase them not merging and reading or having a few ranges where some are merged and some aren't by validating them.

Yeah already in my TODO. Thanks.

Also, are both minSeek and maxReadSize going to be configurable? I assume they are hardcoded just so you can test them out..
Yes I will be making them configurable in consecutive PR's

Also Please refer to the new PR now. #3904

steveloughran · 2022-05-04T12:18:03Z

now this is in the feature branch, this can be closed

mukund-thakur added the enhancement label Sep 29, 2021

mukund-thakur marked this pull request as draft September 29, 2021 11:48

steveloughran reviewed Sep 29, 2021

View reviewed changes

bogthe reviewed Oct 19, 2021

View reviewed changes

hadoop-common-project/benchmark/src/main/java/org/apache/hadoop/benchmark/AsyncBenchmark.java Outdated Show resolved Hide resolved

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java Show resolved Hide resolved

mukund-thakur marked this pull request as ready for review December 2, 2021 07:14

omalley and others added 7 commits December 2, 2021 12:53

async api to throw IOE and basic S3A implementation

06d11e7

Vectored Read API spec

7970faa

Adding contract tests for vectored read API

fd20570

Move benchmark test to hadoop-tools module

65edb35

Review comments

8b8dff8

Merging of ranges in S3A vectored read implementation

fa3ebc1

mukund-thakur force-pushed the HADOOP-11867-vectored-io branch from 1a03999 to fa3ebc1 Compare December 2, 2021 09:55

mukund-thakur requested a review from omalley December 3, 2021 07:50

mukund-thakur commented Dec 3, 2021

View reviewed changes

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/fsdatainputstream.md Outdated Show resolved Hide resolved

steveloughran self-assigned this Dec 3, 2021

mukund-thakur added 2 commits December 6, 2021 12:00

Implementing change detection for vectored reads in S3a

f5180f3

More tests and javadoc

e084602

mukund-thakur added fs fs/s3 changes related to hadoop-aws; submitter must declare test endpoint labels Dec 7, 2021

mukund-thakur added 4 commits December 23, 2021 15:31

Cleaning the s3objets else connections were getting exhausted

9fd804c

Adding retries in getS3Object

b78dded

Fix vectored read for bigger size files

38dcd39

Test for bigger file size

dd3914b

steveloughran reviewed Jan 12, 2022

View reviewed changes

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java Show resolved Hide resolved

steveloughran reviewed Jan 12, 2022

View reviewed changes

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java Show resolved Hide resolved

mehakmeet reviewed Jan 18, 2022

View reviewed changes

mukund-thakur mentioned this pull request Jan 19, 2022

HADOOP-11867. Add a high-performance vectored read API. #3904

Merged

4 tasks

steveloughran closed this May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HADOOP-11867. Add a high performance vectored read API to file system. #3499

HADOOP-11867. Add a high performance vectored read API to file system. #3499

mukund-thakur commented Sep 29, 2021 •

edited

Loading

mukund-thakur commented Sep 29, 2021

steveloughran commented Sep 29, 2021

bogthe left a comment

steveloughran commented Jan 6, 2022

steveloughran left a comment

steveloughran Dec 3, 2021

mukund-thakur Jan 17, 2022

steveloughran Jan 12, 2022

steveloughran commented Jan 12, 2022

mehakmeet left a comment

mukund-thakur commented Jan 19, 2022 •

edited

Loading

steveloughran commented May 4, 2022

HADOOP-11867. Add a high performance vectored read API to file system. #3499

HADOOP-11867. Add a high performance vectored read API to file system. #3499

Conversation

mukund-thakur commented Sep 29, 2021 • edited Loading

Description of PR

How was this patch tested?

For code changes:

mukund-thakur commented Sep 29, 2021

steveloughran commented Sep 29, 2021

bogthe left a comment

Choose a reason for hiding this comment

steveloughran commented Jan 6, 2022

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran Dec 3, 2021

Choose a reason for hiding this comment

mukund-thakur Jan 17, 2022

Choose a reason for hiding this comment

steveloughran Jan 12, 2022

Choose a reason for hiding this comment

steveloughran commented Jan 12, 2022

mehakmeet left a comment

Choose a reason for hiding this comment

mukund-thakur commented Jan 19, 2022 • edited Loading

steveloughran commented May 4, 2022

mukund-thakur commented Sep 29, 2021 •

edited

Loading

mukund-thakur commented Jan 19, 2022 •

edited

Loading