alamb commented on code in PR #828:
URL:
https://github.com/apache/arrow-rs-object-store/pull/828#discussion_r3816883679
##########
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:
I think we should add new test coverage rather than changing existing
coverage -- that way it is easier to verify that this PR does not change
existing behavior
##########
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:
we lost this comment with the reference to the URL format documentation --
can you please restore it?
##########
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:
can you please add a comment here explaining where this format comes from I
think it is
https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html
##########
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:
I double checked that the AWS docs do say that dual-stack is only available
in some regions
> Dual-stack endpoints are available for some services in some regions.
https://docs.aws.amazon.com/sdkref/latest/guide/feature-endpoints.html
--
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]
