Set S3 endpoint region and support OCI object storage #14134
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14134 +/- ##
============================================
- Coverage 43.25% 43.08% -0.18%
- Complexity 9209 9294 +85
============================================
Files 1428 1437 +9
Lines 82577 83676 +1099
Branches 9987 10131 +144
============================================
+ Hits 35717 36049 +332
- Misses 43894 44636 +742
- Partials 2966 2991 +25
Continue to review full report at Codecov.
|
LuQQiu
changed the title
|
@yuzhu PTAL, thanks! |
| .standard() | ||
| .withCredentials(credentials) | ||
| .withClientConfiguration(clientConf) | ||
| .withPathStyleAccessEnabled(Boolean.parseBoolean(conf |
There was a problem hiding this comment.
I think we don't want to override whatever is the default if withPathStyleAccessEnabled is not called.
So ideally, not even call withPathStyleAccessEnabled when Boolean.parseBoolean(conf.get(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS) is false.
There was a problem hiding this comment.
Sounds good! Only set when alluxio conf is set that property, PTAL, thanks
| String region; | ||
| if (conf.isSet(PropertyKey.UNDERFS_S3_ENDPOINT_REGION)) { | ||
| region = conf.get(PropertyKey.UNDERFS_S3_ENDPOINT_REGION); | ||
| } else if (!ServiceUtils.isS3USStandardEndpoint(endpoint)) { |
There was a problem hiding this comment.
Could you explain this a little bit more in comments?
There was a problem hiding this comment.
Yeah, reorganize the code to make it more clear.
If ends with s3.amazonaws.com, that's the standard endpoint with default region so no need to set region
|
@ggezer Thanks for the review! Addressed all the comments, PTAL |
| .get(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS))); | ||
| .withClientConfiguration(clientConf); | ||
|
|
||
| if (conf.isSet(PropertyKey.UNDERFS_S3_DISABLE_DNS_BUCKETS)) { |
There was a problem hiding this comment.
It'll always be set because it has a default value defined. So this if statement is redundant.
Earlier we were calling withPathStyleAccessEnabled only if it was set and true.
|
@ggezer Addressed the comments, PTAL, thanks! |
|
alluxio-bot, merge this please |
What changes are proposed in this pull request?
Fixes #14133
Why are the changes needed?
Solves the issue that S3 cannot recognize endpoint region and supports OCI object storage
Does this PR introduce any user facing changes?
add an s3 region property key