-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-17074 Optimise s3a Listing to be fully asynchronous. #2207
HADOOP-17074 Optimise s3a Listing to be fully asynchronous. #2207
Conversation
Performance result using new test: We can see an improvement of 4s with these configs.
It is evident from the logs that without the improvements, listStatus and listFiles took same time. |
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.
When my IOStatistics patch goes in the stats could be logged here
Overall, I like this. Tangible benefits once you start doing some milliseconds of work per file. You were testing remotely, correct? So list time may be pessimistic. But then again, versioned buckets under load with many tombstones may be worse.
Test wise -Use WriteOperationHelper via getWriteOperationHelper()
, no need to make something else visible,
Production code: I'd rather the new async submit code when into ListOperationCallbacks. That is: no new methods in S3AFilesystem, just the ListOperationsCallbacksImpl taking on more of the work. This stops the FS itself growing.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Listing.java
Outdated
Show resolved
Hide resolved
@@ -1956,6 +1956,14 @@ protected S3ListResult listObjects(S3ListRequest request) throws IOException { | |||
} | |||
} | |||
|
|||
protected CompletableFuture<S3ListResult> listObjectsAsync(S3ListRequest request) { |
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.
make private unless someone needs to get at these in mockito tests
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Outdated
Show resolved
Hide resolved
...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3ADirectoryPerformance.java
Outdated
Show resolved
Hide resolved
...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3ADirectoryPerformance.java
Outdated
Show resolved
Hide resolved
...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3ADirectoryPerformance.java
Show resolved
Hide resolved
...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3ADirectoryPerformance.java
Outdated
Show resolved
Hide resolved
Fixed all review comments and re ran the new test. All good. |
🎊 +1 overall
This message was automatically generated. |
LGTM. +1 pending the changes needed to get checkstyle to be (mostly) quiet nice bit of work here. |
...LGTM, let's see what Yetus says |
🎊 +1 overall
This message was automatically generated. |
Contributed by Mukund Thakur. Change-Id: I1b0574a0c9ebc0805f285dd5280a00e5add081f1
…e#2207) Contributed by Mukund Thakur. Change-Id: Iad9832ee75370a1ba289455c91ea0ef65f6a8286
Tested using ap-south-1 bucket. All good apart from known failures.
https://issues.apache.org/jira/browse/HADOOP-17192
https://issues.apache.org/jira/browse/HADOOP-17190