JonasJ-ap commented on code in PR #5684:
URL: https://github.com/apache/iceberg/pull/5684#discussion_r961282897
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -735,4 +765,15 @@ private Set<Tag> toTags(Map<String, String> properties,
String prefix) {
public Map<String, String> s3BucketToAccessPointMapping() {
return s3BucketToAccessPointMapping;
}
+
+ public <T extends S3ClientBuilder> void applyS3Configuration(T builder) {
Review Comment:
Thank you for your suggestions. Here is what I thought after investigating
the code:
1. `configureEndpoint` must take different endpoints for different type of
client. Hence, moving it to `AwsProperties` cannot eliminate the second param.
I suggest leaving this method unchanged.
2. `credentialsProvider` must take different id, key, token for different
clients. Also, the logic here is more relevant to `AwsClientFactories`. I
suggest leaving this method unchanged.
3. `configureHttpClient` can be moved to `AwsProperties`. The benefit of the
movement is that we can move the `httpClientType` field to `AwsProperties` so
that we do not have to parse it separately in every client factory.
However, the logic of the method seems to make it more suitable to put in
the original place.
I'll try the movement of `configureHttpClient` and see if it makes the code
structure better.
--
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]
