-
Notifications
You must be signed in to change notification settings - Fork 2.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
[ALLUXIO-2397] Add support for alluxio with Minio. #4217
Conversation
Hi @harshavardhana, thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. |
Running checks...
Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks |
Can one of the admins verify this patch? |
You did it @harshavardhana! Thank you for signing the Contribution License Agreement. |
40f32c0
to
fe9e063
Compare
Running checks...
Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks |
fe9e063
to
b16239c
Compare
Running checks...
Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks |
alluxio-bot, check this please |
Running checks...
All checks passed! |
Another sample config without setting up a local Minio instance. One can use our public server.
|
ok to test |
Jenkins, test this please |
Merged build finished. Test PASSed. |
Test PASSed. |
Running checks...
All checks passed! |
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.
@harshavardhana Thanks! I left some comments.
} | ||
String accountOwner = owner == null ? accountOwnerId : owner; | ||
// Default to readable and writable by the user. | ||
short bucketMode = (short) 0 | (short) 0500 | (short) 0200; |
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 is this written this way? Is this equivalent to 700
?
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.
yes it is supposed 700
@@ -149,29 +152,56 @@ public static S3AUnderFileSystem createInstance(AlluxioURI uri) { | |||
} | |||
|
|||
AmazonS3Client amazonS3Client = new AmazonS3Client(credentials, clientConf); | |||
// Set your endpoint. |
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.
NIT: instead of saying "your" you can say "Set the custom endpoint".
Also, could you put that within the if
statement?
if (Configuration.containsKey(PropertyKey.UNDERFS_S3_ENDPOINT)) { | ||
amazonS3Client.setEndpoint(Configuration.get(PropertyKey.UNDERFS_S3_ENDPOINT)); | ||
} | ||
// Set your region, primary useful for s3 compliant endpoints |
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.
NIT: don't use "your". Also, move it within the if
block.
// Set your region, primary useful for s3 compliant endpoints | ||
// with custom regions. | ||
if (Configuration.containsKey(PropertyKey.UNDERFS_S3A_REGION)) { | ||
if (Configuration.get(PropertyKey.UNDERFS_S3A_REGION).equals("us-east-1")) { |
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 is us-east-1
special? what about other regions?
amazonS3Client.setRegion(usEast1); | ||
} | ||
} | ||
// Disable DNS style buckets, this enables path style primary useful for |
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.
NIT: move this comment within the if
block.
@@ -78,6 +78,8 @@ | |||
UNDERFS_S3_THREADS_MAX(Name.UNDERFS_S3_THREADS_MAX, 22), | |||
UNDERFS_S3_UPLOAD_THREADS_MAX(Name.UNDERFS_S3_UPLOAD_THREADS_MAX, 2), | |||
UNDERFS_S3A_CONSISTENCY_TIMEOUT_MS(Name.UNDERFS_S3A_CONSISTENCY_TIMEOUT_MS, 60000), | |||
UNDERFS_S3A_REGION(Name.UNDERFS_S3A_REGION, "us-east-1"), |
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.
Previously, this did not exist. Therefore, shouldn't the default be null or empty?
b16239c
to
cfb6bd7
Compare
@gpang thanks for your review. Addressed your comments.. Don't need REGION option so removed it one less option. |
Merged build finished. Test FAILed. |
Test FAILed. Build result: FAILURE[...truncated 1235 lines...][JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/s3/target/alluxio-underfs-s3-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-s3/1.4.0-SNAPSHOT/alluxio-underfs-s3-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/assembly/pom.xml to org.alluxio/alluxio-assemblies/1.4.0-SNAPSHOT/alluxio-assemblies-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/shell/pom.xml to org.alluxio/alluxio-shell/1.4.0-SNAPSHOT/alluxio-shell-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/pom.xml to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/target/alluxio-underfs-oss-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/target/alluxio-underfs-oss-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/client/pom.xml to org.alluxio/alluxio-keyvalue-client/1.4.0-SNAPSHOT/alluxio-keyvalue-client-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/pom.xml to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/target/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/target/alluxio-underfs-hdfs-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/pom.xml to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/target/alluxio-underfs-swift-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/target/alluxio-underfs-swift-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/s3a/pom.xml to org.alluxio/alluxio-underfs-s3a/1.4.0-SNAPSHOT/alluxio-underfs-s3a-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/examples/pom.xml to org.alluxio/alluxio-examples/1.4.0-SNAPSHOT/alluxio-examples-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/core/server/pom.xml to org.alluxio/alluxio-core-server/1.4.0-SNAPSHOT/alluxio-core-server-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/common/pom.xml to org.alluxio/alluxio-keyvalue-common/1.4.0-SNAPSHOT/alluxio-keyvalue-common-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/pom.xml to org.alluxio/alluxio-underfs/1.4.0-SNAPSHOT/alluxio-underfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/pom.xml to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/target/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/target/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/integration/pom.xml to org.alluxio/alluxio-integration/1.4.0-SNAPSHOT/alluxio-integration-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/pom.xml to org.alluxio/alluxio-keyvalue/1.4.0-SNAPSHOT/alluxio-keyvalue-1.4.0-SNAPSHOT.pomchannel stoppedTest FAILed. |
cfb6bd7
to
6115e4a
Compare
Build failure fixed. |
Merged build finished. Test PASSed. |
Test PASSed. |
@@ -411,7 +414,8 @@ | |||
"alluxio.underfs.s3.admin.threads.max"; | |||
public static final String UNDERFS_S3_DISABLE_DNS_BUCKETS = | |||
"alluxio.underfs.s3.disable.dns.buckets"; | |||
public static final String UNDERFS_S3_ENDPOINT = "alluxio.underfs.s3.endpoint"; | |||
public static final String UNDERFS_S3_ENDPOINT = | |||
"alluxio.underfs.s3.endpoint"; |
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.
Could you revert the formatting change here?
if (Configuration.get(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS).equals("true")) { | ||
S3ClientOptions clientOptions = new S3ClientOptions(); | ||
clientOptions.withPathStyleAccess(true); | ||
amazonS3Client.setS3ClientOptions(clientOptions); |
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.
I think the recommended way is to use the Builder pattern.
if (Configuration.containsKey(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS)) { | ||
// Disable DNS style buckets, this enables path style primary useful for | ||
// s3 compliant endpoints. | ||
if (Configuration.get(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS).equals("true")) { |
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.
Could use Configuration.getBoolean
here
String accountOwner = ""; // There is no known account owner by default. | ||
// if ACL enabled inherit bucket acl for all the objects. | ||
if (Configuration.containsKey(PropertyKey.UNDERFS_S3A_ENABLE_ACL)) { | ||
if (Configuration.get(PropertyKey.UNDERFS_S3A_ENABLE_ACL).equals("true")) { |
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.
Could use Configuration.getBoolean
here
if (Configuration.containsKey(PropertyKey.UNDERFS_S3_ENDPOINT)) { | ||
amazonS3Client.setEndpoint(Configuration.get(PropertyKey.UNDERFS_S3_ENDPOINT)); | ||
} | ||
if (Configuration.containsKey(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS)) { |
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.
Since you have already set a default value for the key, its unnecessary to check the existence.
short bucketMode = (short) 700; | ||
String accountOwner = ""; // There is no known account owner by default. | ||
// if ACL enabled inherit bucket acl for all the objects. | ||
if (Configuration.containsKey(PropertyKey.UNDERFS_S3A_ENABLE_ACL)) { |
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.
Since you have already set a default value for the key, its unnecessary to check the existence.
Merged build finished. Test PASSed. |
Test PASSed. |
@@ -398,6 +399,7 @@ | |||
"alluxio.underfs.oss.connection.timeout.ms"; | |||
public static final String UNDERFS_OSS_CONNECT_TTL = "alluxio.underfs.oss.connection.ttl"; | |||
public static final String UNDERFS_OSS_SOCKET_TIMEOUT = "alluxio.underfs.oss.socket.timeout.ms"; | |||
public static final String UNDERFS_S3A_ENABLE_ACL = "alluxio.underfs.s3a.enable.acl"; |
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.
Could you document this new configuration option? Also, this may be better named like inherit_acl
.
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.
Where would you document this?
if (Configuration.containsKey(PropertyKey.UNDERFS_S3_ENDPOINT)) { | ||
amazonS3Client.setEndpoint(Configuration.get(PropertyKey.UNDERFS_S3_ENDPOINT)); | ||
} | ||
// Disable DNS style buckets, this enables path style requests. | ||
if (Configuration.getBoolean(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS)) { |
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.
Could you set the default value of this property in PropertyKey
to be false
?
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.
sure. I think @gpang asked me to keep it as "null"
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.
I think he was referring to the new region field you added. Since all users of this property (s3n/s3a) default to false when it is empty, we should be able to use a default value.
@harshavardhana Two minor comments otherwise this looks good. |
77f074d
to
2850248
Compare
Merged build finished. Test PASSed. |
Test PASSed. |
2850248
to
c749734
Compare
Done documented. Let me know the wording @calvinjia |
Merged build finished. Test FAILed. |
Test FAILed. Failed Tests: 1org.alluxio:alluxio-tests: 1Test FAILed. |
Is this related to my patch?
|
Jenkins, test this please. |
Jenkins, test this please |
Merged build finished. Test PASSed. |
Test PASSed. |
LGTM |
Is there anything else pending on us for this patch @gpang @calvinjia ? |
alluxio.underfs.s3a.consistency.timeout.ms,60000 | ||
alluxio.underfs.s3a.direct.writes.enabled,true | ||
alluxio.underfs.s3a.secure.http.enabled,false | ||
alluxio.underfs.s3a.server.side.encryption.enabled,false | ||
alluxio.underfs.s3a.socket.timeout.ms,50000 | ||
alluxio.underfs.s3a.enable.acl,true |
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.
Could you update this value to match the new one?
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.
Ah sorry, doing that right away.
This patch brings in two new options to `alluxio-site.properties` - UNDERFS_S3A_ENABLE_ACL to disable inheriting bucket ACLs This patch also adds support for s3a driver to honor disabling `dns.buckets` A sample configuration looks like this Fixes https://alluxio.atlassian.net/browse/ALLUXIO-2397 ``` alluxio.underfs.address=s3a://testbucket/ alluxio.underfs.s3.disable.dns.buckets=true alluxio.underfs.s3.endpoint=http://localhost:9000 alluxio.underfs.s3a.region="us-east-1" alluxio.underfs.s3a.enable.acl=false aws.accessKeyId=USWUXHGYZQYFYFFIT3RE aws.secretKey=MOJRH0mkL1IPauahWITSVvyDrQbEEIwljvmxdq03 ```
c749734
to
d4948ef
Compare
Done @calvinjia |
Merged build finished. Test PASSed. |
Test PASSed. |
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
This patch brings in two new options to
alluxio-site.properties
This patch also adds support for s3a driver to honor disabling
dns.buckets
A sample configuration looks like this
Fixes https://alluxio.atlassian.net/browse/ALLUXIO-2397