AWS: create HttpClientProperties, move s3 related methods into S3FileIOProperties #7562
Conversation
| * use this provider to get AWS credentials provided instead of reading the default credential | ||
| * chain to get AWS access credentials. | ||
| */ | ||
| public static final String CLIENT_CREDENTIALS_PROVIDER = "client.credentials-provider"; |
There was a problem hiding this comment.
this should not be in S3FileIOProperties, this is generic configuration for all AWS clients, thus the prefix client.. So we could create a class AwsClientProperties for them
There was a problem hiding this comment.
Thanks @akshayakp97, I reviewed the code change and had some comments but didn't get a chance to review the tests. I'll take a look at the tests when I get a chance
| public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_APACHE; | ||
| /** | ||
| * @deprecated will be removed in 1.4.0, use {@link org.apache.iceberg.aws.HttpClientProperties} | ||
| * instead |
There was a problem hiding this comment.
The indentation for instead on the newline (and in all the other places) looks off to me, is this how spotless is formatting it?
| try { | ||
| try { | ||
| provider = | ||
| DynMethods.builder("create") | ||
| .hiddenImpl(providerClass, Map.class) | ||
| .buildStaticChecked() | ||
| .invoke(clientCredentialsProviderProperties); | ||
| } catch (NoSuchMethodException e) { | ||
| provider = | ||
| DynMethods.builder("create").hiddenImpl(providerClass).buildStaticChecked().invoke(); | ||
| } | ||
|
|
||
| return provider; | ||
| } catch (NoSuchMethodException e) { | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| "Cannot create an instance of %s, it does not contain a static 'create' or 'create(Map<String, String>)' method", | ||
| credentialsProviderClass), | ||
| e); | ||
| } |
There was a problem hiding this comment.
This block is a bit hard to read imo, I got what's going on but while we're at this refactoring we can take the opportunity to do some cleanup. For this could we have a separate helper for obtaining the provider with some inline comment on our "fallback" behavior.
| @SuppressWarnings("checkstyle:HiddenField") | ||
| public AwsCredentialsProvider credentialsProvider( | ||
| String accessKeyId, String secretAccessKey, String sessionToken) { | ||
| if (accessKeyId != null) { |
There was a problem hiding this comment.
and secretAccessKey not null
There was a problem hiding this comment.
Maybe we should use Strings.isNullOrEmpty for all these
There was a problem hiding this comment.
secretAccessKey was missing here - https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java#L1695
ill update it
There was a problem hiding this comment.
yeah, that's probably a miss we can fix here
| "Cannot initialize %s, it does not implement %s.", | ||
| credentialsProviderClass, AwsCredentialsProvider.class.getName())); | ||
|
|
||
| // try to invoke 'create(Map<String, String>)' static method, otherwise fallback to 'create()' |
There was a problem hiding this comment.
once javadoc is added, this can be removed
| } | ||
|
|
||
| @SuppressWarnings("checkstyle:HiddenField") | ||
| private <T extends SdkClientBuilder> void configureEndpoint(T builder, String endpoint) { |
There was a problem hiding this comment.
can we just merge this method to applyEndpointConfigurations?
| } | ||
|
|
||
| /** | ||
| * Tries to first dynamically load the credentials provider class. If successful, try to invoke |
There was a problem hiding this comment.
We don't typically add javadoc for private methods, this can be moved as a part of the public method doc to explain how the credential provider dynamic loading works.
Subtask of #7516
This PR creates separate class
HttpClientProperties. Also, move all s3 related methods intoS3FileIOPropertiessince we plan to deprecateAwsProperties.As part of #7156, when
S3FileIOAwsClientFactoryis added for creating S3 client, we will use methods fromS3FileIOPropertiesandHttpClientProperties.Ran integ tests -
TestS3FileIOIntegration,TestS3MultipartUploadandTestDefaultAwsClientFactory