[AWS] S3FileIO - Add Cross-Region Bucket Access #9804
Conversation
| ArgumentCaptor.forClass(S3Configuration.class); | ||
|
|
||
| Mockito.doReturn(mockA).when(mockA).dualstackEnabled(Mockito.anyBoolean()); | ||
| Mockito.doReturn(mockA).when(mockA).crossRegionAccessEnabled(Mockito.anyBoolean()); |
There was a problem hiding this comment.
please also add Assertions.assertThat(s3Configuration.crossRegionAccessEnabled()).isTrue() at the bottom of this test method
There was a problem hiding this comment.
@nastra
crossRegionAccessEnabled is an S3Client attribute rather than S3Configuration (same as dualstackEnabled).
Similar to dualstackEnabled attribute (not asserted in this test method), s3Configuration.crossRegionAccessEnabled() does not exist.
There was a problem hiding this comment.
Yeah unfortunately I think getting the value of the attributes from the client isn't possible without doing some reflection hacks.
What I think we can do is add S3 integration tests though to verify the actual cross region access behavior. That seems like a more useful test to verify that when the property is set we can perform operations across regions as expected. cc @jackye1995 @geruh @rahil-c
There was a problem hiding this comment.
+1 for integ test, there is already a AWS_CROSS_REGION config there that can be used
There was a problem hiding this comment.
There's also a AWS_TEST_CROSS_REGION_BUCKET config (although originally added for S3 Access Point tests).
Can anybody help / provide guidance on this feature's integration tests? Thanks.
| * Determines if S3 client will allow Cross-Region bucket access, default to false. | ||
| * | ||
| * <p>For more details, see | ||
| * https://docs.aws.amazon.com/AmazonS3/latest/userguide/dual-stack-endpoints.html |
There was a problem hiding this comment.
is this a wrong doc link?
docs/docs/aws.md
Outdated
|
|
||
| S3 Cross-Region bucket access can be turned on by setting catalog property `s3.cross-region-access-enabled` to `true`. | ||
| This is turned off by default to avoid first S3 API call increased latency. | ||
| For more details, please refer to [Cross-Region access for Amazon S3 |
There was a problem hiding this comment.
Can we follow the convention of the other sections and add an example here?
Something like:
spark-sql --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCatalog \
--conf spark.sql.catalog.my_catalog.warehouse=s3://my-bucket2/my/key/prefix \
--conf spark.sql.catalog.my_catalog.type=glue \
--conf spark.sql.catalog.my_catalog.io-impl=org.apache.iceberg.aws.s3.S3FileIO \
--conf spark.sql.catalog.my_catalog.s3.cross-region-access-enabled=true
There was a problem hiding this comment.
I did the same as in S3 Access Control List and and S3 Write Checksum Verification sections, since it's a single parameter configuration. But I can definitely change it.
|
Hi, what is blocking from merging this change? |
|
+1 to adding this support @ebelgasmi12 @nastra @amogh-jahagirdar @jackye1995 |
|
sounds like a resonable change to add, for adding an integ test, which is what this pr is pending please check this pr
|
|
@elmehdibelgasmi |
|
@singhpk234 Created separate PR for Spec update #11260 |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
9 participants
[AWS] S3FileIO - Add Cross-Region Bucket Access
Made corresponding updates to
mainandtest.Resolves #9785
CC @nastra