HADOOP-19286: S3A: Support cross region access when S3 region/endpoint is set #7067
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
@ahmarsuhail @steveloughran Could you please review the changes ? |
steveloughran
requested changes
Sep 24, 2024
| final Configuration newConf = new Configuration(getConfiguration()); | ||
| // skip the test if the region is eu-west-2 | ||
| String region = getFileSystem().getS3AInternals().getBucketMetadata().bucketRegion(); | ||
| if (EU_WEST_2.equals(region)) { |
There was a problem hiding this comment.
- I'd like a different region here a that is my region and I would like the test coverage.
- what happens with third party stores?
There was a problem hiding this comment.
- Ack.
- As per the doc (https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/s3-cross-region.html)
When you reference an existing bucket in a request, such as when you use the putObject method, the SDK initiates a request to the Region configured for the client.
If the bucket does not exist in that specific Region, the error response includes the actual Region where the bucket resides. The SDK then uses the correct Region in a second request.
To optimize future requests to the same bucket, the SDK caches this Region mapping in the client.
So as per the implementation, It looks like it won't be supported by thirdparty store and each store have to implement it separately.
| // enable cross region access | ||
| newConf.setBoolean(AWS_S3_CROSS_REGION_ACCESS_ENABLED, true); | ||
| newConf.set(AWS_REGION, EU_WEST_2); | ||
| S3AFileSystem fs = new S3AFileSystem(); |
There was a problem hiding this comment.
needs to be in try-with-resources to close()
| final Configuration newConf = new Configuration(getConfiguration()); | ||
| // enable cross region access | ||
| newConf.setBoolean(AWS_S3_CROSS_REGION_ACCESS_ENABLED, true); | ||
| newConf.set(AWS_REGION, EU_WEST_2); |
There was a problem hiding this comment.
same comments as above
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Sep 25, 2024
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran - I have addressed your comments regarding the integration tests. |
steveloughran
requested changes
Sep 30, 2024
There was a problem hiding this comment.
If we are dynamically doing a probe, actually the reading comes back as "null" hadoop s3guard bucket-info prints this (tested with two different stores)
Bucket metadata from S3
Region null
Location Name null
Location Type null
I had in mind just having a constant string which we could check in the configuration; what do you have done discovers the information later, but it looks for that region==null actually it is more reliable.
Please change the crow to look for null value. Having the new constant is good as then we can declare that this is what you said to be excluded from other tests (conditional-write?) (actually, thinking of it, maybe we should have a feature flag enum. Not in this PR though.)
| */ | ||
| private void skipCrossRegionTest() throws IOException { | ||
| String region = getFileSystem().getS3AInternals().getBucketMetadata().bucketRegion(); | ||
| if (SA_EAST_1.equals(region) || NON_AWS_REGION.equals(region)) { |
There was a problem hiding this comment.
please change the non-aws probe for just region == null
|
💔 -1 overall
This message was automatically generated. |
|
The error seems unrelated. |
steveloughran
changed the title
@steveloughran Sure, it was in my todo list, but missed somehow. Thanks for pointing this out. I have done the doc changes in |
|
🎊 +1 overall
This message was automatically generated. |
|
thanks. can you do a PR for the backport and rerun the tests, then I will merge |
shameersss1
added a commit
to shameersss1/hadoop
that referenced
this pull request
Oct 2, 2024
…t is set (apache#7067) Adds new option s3a.cross.region.access.enabled Which is true by default This enables cross region access as a separate config and enable/disables it irrespective of region/endpoint is set. Contributed by Syed Shameerur Rahman
|
Thanks a lot @steveloughran for the detailed review and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently when S3 region nor endpoint is set, the default region is set to us-east-2 with cross region access enabled. But when region or endpoint is set, cross region access is not enabled.
The proposal here is to carves out cross region access as a separate config and enable/disable it irrespective of region/endpoint is set. This gives more flexibility to the user.
S3 cross region access can be enabled/disabled via config
fs.s3a.cross.region.access.enabledwhich is set to true by default.Tested with us-east-1