jackye1995 commented on pull request #1844: URL: https://github.com/apache/iceberg/pull/1844#issuecomment-747943701
@danielcweeks since we are approaching the next release and also the holiday season, I would like to minimize the change here and only focus on finalizing the client factory interface, so at least this PR can join the current release train. I have moved AssumeRole part out of the picture and will use another PR for that discussion. I agree that it would be cleaner to move the factory out of the `AwsProperties` class. I have created `AwsClientFactories` class for that, following a similar structure as the`LocationProviders` class and made `DefaultAwsClientFactory` an inner class. I think this should make the whole thing much cleaner. The only discussion at this point I believe is about making `AwsClientFactory` serializable or not. I tried about not serializing it, and as a result in S3FileIO I have to do: ```java this.s3 = () -> AwsClientFactories.from(properties)::s3 ``` In that call above, `properties` got serialized as a part of the serializable supplier, which defeats the purpose of us using `AwsProperties` to store only useful information. So I think making `AwsClientFactory` serializable should still be the way to go there. Please let me know what you think, thank you! ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
