rajarshisarkar commented on code in PR #5684:
URL: https://github.com/apache/iceberg/pull/5684#discussion_r970795907
##########
aws/src/main/java/org/apache/iceberg/aws/AssumeRoleAwsClientFactory.java:
##########
@@ -34,49 +32,54 @@
import software.amazon.awssdk.services.sts.StsClient;
import
software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.Tag;
public class AssumeRoleAwsClientFactory implements AwsClientFactory {
private String roleArn;
private String externalId;
- private Set<Tag> tags;
private int timeout;
private String region;
- private String s3Endpoint;
- private boolean s3UseArnRegionEnabled;
- private String dynamoDbEndpoint;
- private String httpClientType;
+ private AwsProperties awsProperties;
+ private AssumeRoleRequest assumeRoleRequest;
@Override
public S3Client s3() {
return S3Client.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, s3Endpoint))
- .serviceConfiguration(s ->
s.useArnRegionEnabled(s3UseArnRegionEnabled).build())
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyS3EndpointConfigurations)
+ .applyMutation(awsProperties::applyS3ServiceConfigurations)
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().applyMutation(this::configure).build();
+ return GlueClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public KmsClient kms() {
- return KmsClient.builder().applyMutation(this::configure).build();
+ return KmsClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public DynamoDbClient dynamo() {
return DynamoDbClient.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, dynamoDbEndpoint))
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyDynamoDbEndpointConfigurations)
.build();
}
@Override
public void initialize(Map<String, String> properties) {
+ this.awsProperties = new AwsProperties(properties);
Review Comment:
[doubt] Can some of the instance variables like `roleArn`, `externalId`, and
`timeout` be local variables in `initialize()` if they are only required to
create the `assumeRoleRequest`.
##########
aws/src/main/java/org/apache/iceberg/aws/AssumeRoleAwsClientFactory.java:
##########
@@ -34,49 +32,54 @@
import software.amazon.awssdk.services.sts.StsClient;
import
software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.Tag;
public class AssumeRoleAwsClientFactory implements AwsClientFactory {
private String roleArn;
private String externalId;
- private Set<Tag> tags;
private int timeout;
private String region;
- private String s3Endpoint;
- private boolean s3UseArnRegionEnabled;
- private String dynamoDbEndpoint;
- private String httpClientType;
+ private AwsProperties awsProperties;
+ private AssumeRoleRequest assumeRoleRequest;
@Override
public S3Client s3() {
return S3Client.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, s3Endpoint))
- .serviceConfiguration(s ->
s.useArnRegionEnabled(s3UseArnRegionEnabled).build())
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyS3EndpointConfigurations)
+ .applyMutation(awsProperties::applyS3ServiceConfigurations)
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().applyMutation(this::configure).build();
+ return GlueClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public KmsClient kms() {
- return KmsClient.builder().applyMutation(this::configure).build();
+ return KmsClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public DynamoDbClient dynamo() {
return DynamoDbClient.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, dynamoDbEndpoint))
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyDynamoDbEndpointConfigurations)
.build();
}
@Override
public void initialize(Map<String, String> properties) {
+ this.awsProperties = new AwsProperties(properties);
Review Comment:
If possible, can we make the below instance variables initialisation from
`awsProperties` itself? Something like:
```
this.awsProperties = new AwsProperties(properties);
...
...
this.roleArn = awsProperties.clientAssumeRoleArn();
this.timeout = awsProperties.clientAssumeRoleTimeout();
...
```
--
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]
