-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-11867: Add gather API to file system. #1830
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet.
- What are the benchmark numbers?
- it's going to need some docs in inputstream.md
- It'd be good to PoC an object store: s3a, ozone, abfs...
- And we will need to think about some contract tests to test/break the implementations
I presume this design will suit ORC?
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* Find the checksum ranges that correspond to the given data ranges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be nice to explain why this is needed, for those of us who don't normally go near this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why what is needed? You mean the code to compare the checksums? The current code requires a lot of context that isn't true in the new API. The current code is super inefficient because it did a bad job of working around those limitations. In particular, if you look at the current pread code, it reopens the crc file for each seek.
* @return the minimum number of bytes | ||
*/ | ||
default int minimumReasonableSeek() { | ||
return 4 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should really be constants somewhere, even if within this interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, although the difference between having the constant in the method versus defined and used once is pretty minor.
Those constants should be determined by testing on each of the different file systems. I suspect the minimum seek on local fs < hdfs < s3.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/PositionedReadable.java
Show resolved
Hide resolved
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/AsyncReaderUtils.java
Show resolved
Hide resolved
...mmon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestAsyncReaderUtils.java
Show resolved
Hide resolved
...mmon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestAsyncReaderUtils.java
Show resolved
Hide resolved
The benchmark numbers are posted on the jira. You'll need to help with the spec that you've developed in fsdatainputstream.md. Fundamentally, the new call is logically the same the input ranges being read using pread in an undefined order. And yes, I believe this structure will work well for ORC (and likely Parquet). |
@omalley -you still working on this? |
7b5a300
to
c988142
Compare
int requestLength = request.getLength(); | ||
// If we need to change the offset or length, make a copy and do it | ||
if (offsetChange != 0 || readData.remaining() != requestLength) { | ||
readData = readData.slice(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new name should be readDataCopy or anything better just to be sure that we are not changing the original buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it isn't copying the data. It is much closer to ByteBuffer's slice, which gives a second view on to the same data buffer. So you get a new ByteBuffer object that shares the same underlying memory.
hey @omalley -thanks for the update. Could you do anything with the fields in AsyncBenchmark, as they are flooding yetus
|
hadoop-common-project/benchmark/src/main/java/org/apache/hadoop/benchmark/AsyncBenchmark.java
Show resolved
Hide resolved
Yeah, I just added a suppression file for findbugs that hopefully will make Yetus happy. Sigh findbugs and generated code are not a good combination. |
💔 -1 overall
This message was automatically generated. |
I am trying to compile and run the benchmark added. I am using this command Also when I try to run the same using IDE while selecting the JRE to be Bundled, it works fine. Anything specific I have to do before running the benchmark. FYI related : https://stackoverflow.com/questions/61267495/exception-in-thread-main-java-lang-nosuchmethoderror-java-nio-bytebuffer-flip Thanks |
@mukund-thakur : build and test with the same JDK; java 9+ added some overloaded methods to bytebuyffer. If code has been built against a newer JVM than the one you test against, you will get link problems. Warning: some openjdk8 builds (Amazon Corretto) have the overloaded methods, so cannot be used to build things you intend to run elsewhere. Recommend you set up JAVA_HOME to point to the java version you want, run maven builds on the command line |
Thank @steveloughran . It works after setting java home explicitly to 1.8. |
I have one question. Why merging of ranges is not done for RawLocalFileSystem but done for ChecksumFileSystem? |
} | ||
stream.readAsync(ranges, bufferChoice.allocate); | ||
for(FileRange range: ranges) { | ||
blackhole.consume(range.getData().get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blackhole.consume(ranges); can be used ?
Add API to PositionedReadable to have an asynchronous gather API.