pdeva commented on code in PR #828:
URL:
https://github.com/apache/arrow-rs-object-store/pull/828#discussion_r3817647560
##########
src/aws/builder.rs:
##########
@@ -1739,6 +1803,7 @@ mod tests {
fn s3_test_bucket_endpoint() {
let builder = AmazonS3Builder::new()
.with_endpoint("http://some.host:1234";)
+ .with_use_dualstack_endpoint(true)
Review Comment:
Addressed in
[`d125913`](https://github.com/apache/arrow-rs-object-store/commit/d1259133d6179ab17ddf857ba26b326da2e1efb1):
I restored the existing `test_parse_bucket_az`, `s3_test_bucket_endpoint`, and
S3 endpoint-precedence coverage unchanged, and moved the dual-stack override
assertions into the new `test_generated_s3_endpoints` matrix.
##########
src/aws/builder.rs:
##########
@@ -1244,15 +1274,15 @@ impl AmazonS3Builder {
)) as _
};
+ let use_dualstack_endpoint = self.use_dualstack_endpoint.get()?;
let (session_provider, zonal_endpoint) = match self.s3_express.get()? {
true => {
let zone = parse_bucket_az(&bucket).ok_or_else(|| {
let bucket = bucket.clone();
Error::ZoneSuffix { bucket }
})?;
- //
https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-express-Regions-and-Zones.html
Review Comment:
Restored the original S3 Express Regions and Zones documentation reference
immediately above endpoint construction in
[`d125913`](https://github.com/apache/arrow-rs-object-store/commit/d1259133d6179ab17ddf857ba26b326da2e1efb1).
##########
src/aws/builder.rs:
##########
@@ -1278,12 +1308,21 @@ impl AmazonS3Builder {
// If `endpoint` is provided it's assumed to be consistent with
`virtual_hosted_style_request` or `s3_express`.
// For example, if `virtual_hosted_style_request` is true then
`endpoint` should have bucket name included.
let virtual_hosted = self.virtual_hosted_style_request.get()?;
+ let dualstack = if use_dualstack_endpoint {
+ ".dualstack"
+ } else {
+ ""
+ };
let bucket_endpoint = match (&endpoint, zonal_endpoint,
virtual_hosted) {
(Some(endpoint), _, true) => endpoint.clone(),
(Some(endpoint), _, false) => format!("{}/{}",
endpoint.trim_end_matches("/"), bucket),
(None, Some(endpoint), _) => endpoint,
- (None, None, true) =>
format!("https://{bucket}.s3.{region}.amazonaws.com";),
- (None, None, false) =>
format!("https://s3.{region}.amazonaws.com/{bucket}";),
+ (None, None, true) => {
Review Comment:
Added the AWS Virtual Hosting documentation reference above the generated
path-style and virtual-hosted endpoint formats in
[`d125913`](https://github.com/apache/arrow-rs-object-store/commit/d1259133d6179ab17ddf857ba26b326da2e1efb1).
##########
src/aws/builder.rs:
##########
@@ -961,6 +981,16 @@ impl AmazonS3Builder {
self
}
+ /// Configure whether to use dual-stack endpoints
+ ///
+ /// Defaults to `false`. If enabled for a service or region without a
dual-stack endpoint,
Review Comment:
Thanks for confirming. I linked the AWS dual-stack endpoint configuration
documentation directly from `with_use_dualstack_endpoint` in
[`d125913`](https://github.com/apache/arrow-rs-object-store/commit/d1259133d6179ab17ddf857ba26b326da2e1efb1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
