-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-16801: S3Guard-listFiles will not query s3 if all listings are… #1815
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
Conversation
ac9ea61 to
6b742c9
Compare
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
left a comment
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.
thanks for this, it's going to take some careful work from both of us to merge in, but at a quick glance it makes sense.
It's going to have to wait for #1810, which is changing auth mode marking on dirs, and fixes a major problem which has surfaced. The changes will mean tweaks to your test, but they don't step near the production code you've been editing.
made some minor comments as I briefly looked at the initial PR
Reminder: we need to know which S3 endpoint you ran against. We all have to live by that rule.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3Guard.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/hadoop/fs/s3a/s3guard/ITestDynamoDBMetadataStoreAuthoritativeMode.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/hadoop/fs/s3a/s3guard/ITestDynamoDBMetadataStoreAuthoritativeMode.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/hadoop/fs/s3a/s3guard/ITestDynamoDBMetadataStoreAuthoritativeMode.java
Outdated
Show resolved
Hide resolved
|
Sorry, I had mentioned the testing in the jira ticket, forgot to add here. |
6b742c9 to
252108a
Compare
|
I re-ran only the two tests as I only made changes to them in the subsequent commit. I used s3 us-west-2. |
|
🎊 +1 overall
This message was automatically generated. |
252108a to
f4a8515
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
OK, I've merged in my auth code patch to trunk; you will have to apply yours on top
|
|
@steveloughran |
f4a8515 to
638cbdd
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
Re-ran the tests after rebase. Against s3 us-west-1. Options: "-Dscale -Ds3guard -Dauth -Ddynamo". Only one test fails. I checked the trunk branch. The same test fails consistently on trunk branch too. So not related to my changes: |
bgaborg
left a comment
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.
Thanks for working on this @mustafaiman.
I can see the direction it's going, but I found some things that we need to discuss first.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/OperationCallbacks.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/ImportOperation.java
Outdated
Show resolved
Hide resolved
638cbdd to
e045381
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
re-ran the same tests against s3 us-west-1 |
bgaborg
left a comment
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.
LGTM, +1. Tests were running against Ireland.
|
hey @steveloughran do you have any more comments? Can you merge? |
|
gabor has merged, no comments from me, but I'm going to rebase my ongoing work onto trunk to help validate this. If I see problems in the next few days, I'll raise followup JIRAs. thanks! |
… authoritative (apache#1815). Contributed by Mustafa İman.
…istings are authoritative (apache#1815). Contributed by Mustafa İman. (cherry picked from commit 5977360) Change-Id: Ib5c3517b4b62543614078d213749a3c2f77616f8
… authoritative
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute