amogh-jahagirdar commented on a change in pull request #4239:
URL: https://github.com/apache/iceberg/pull/4239#discussion_r816455677
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsClientFactory.java
##########
@@ -62,4 +63,14 @@
*/
void initialize(Map<String, String> properties);
+ /**
+ * Get http client config
+ */
+ String getHttpClientConfig();
+
+ /**
+ * Get http client builder based on http client config
+ */
+ SdkHttpClient.Builder getHttpClientBuilder();
Review comment:
I'm not sure about this. We allow people to pass in their own
AwsClientFactory instances at runtime (defaulting to the default one if they
don't pass it in).
https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java#L53
So if the interface gets changed, and somebody passes in their existing
AwsClientFactory which does not implement these 2 methods it would break. I
think it makes more sense if everything is contained within the
Default/AssumeRoleAwsClientFactory (people with custom implementations would
anyways be responsible for creating their own clients).
Or if we're aiming for reducing duplication, I suppose we could have a
default implementation of just getHttpClientBuilder() in the interface which
just checks the AwsProperties http client type before returning. I am not sure
if we need a separate method for getting the config, that seems like something
that would be fixed. If we do this I believe we wouldn't need a new
BaseAwsClientFactory class.
Let me know if I missed something!
--
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]
