amogh-jahagirdar commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r911184125
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements
AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
.serviceConfiguration(s3Configuration(s3PathStyleAccess,
s3UseArnRegionEnabled))
.credentialsProvider(credentialsProvider(s3AccessKeyId,
s3SecretAccessKey, s3SessionToken))
.build();
}
@Override
public GlueClient glue() {
- return
GlueClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return GlueClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
I don't think this is right, we're calling configureEndpoint with the
region, instead of the new configureRegion?
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements
AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
.serviceConfiguration(s3Configuration(s3PathStyleAccess,
s3UseArnRegionEnabled))
.credentialsProvider(credentialsProvider(s3AccessKeyId,
s3SecretAccessKey, s3SessionToken))
.build();
}
@Override
public GlueClient glue() {
- return
GlueClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return GlueClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
+ .build();
}
@Override
public KmsClient kms() {
- return
KmsClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return KmsClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
+ .build();
}
@Override
public DynamoDbClient dynamo() {
- return
DynamoDbClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return DynamoDbClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
same
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -280,6 +280,12 @@ public class AwsProperties implements Serializable {
*/
public static final String CLIENT_ASSUME_ROLE_REGION =
"client.assume-role.region";
+ /**
+ * The value must be one of {@link software.amazon.awssdk.regions.Region},
such as 'us-east-1'.
+ * For more details, see
https://docs.aws.amazon.com/general/latest/gr/rande.html
+ */
+ public static final String CLIENT_REGION = "client.region";
Review Comment:
I'm guessing it's just following the existing namespace convention for AWS
clients; unless I'm missing something, I don't think for region it needs to be
qualified in the "client" namespace, so just `region` would suffice.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
