JonasJ-ap commented on code in PR #7066:
URL: https://github.com/apache/iceberg/pull/7066#discussion_r1133317973
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1173,14 +1205,12 @@ public <T extends AwsSyncClientBuilder> void
applyHttpClientConfigurations(T bui
switch (httpClientType) {
case HTTP_CLIENT_TYPE_URLCONNECTION:
UrlConnectionHttpClientConfigurations
urlConnectionHttpClientConfigurations =
- (UrlConnectionHttpClientConfigurations)
-
loadHttpClientConfigurations(UrlConnectionHttpClientConfigurations.class.getName());
+
loadHttpClientConfigurations(UrlConnectionHttpClientConfigurations.class.getName());
Review Comment:
Thank you for refactoring this.
##########
aws/src/test/java/org/apache/iceberg/aws/TestAwsClientFactories.java:
##########
@@ -132,6 +136,41 @@ public void
testLakeFormationAwsClientFactorySerializable() throws IOException {
.isInstanceOf(LakeFormationAwsClientFactory.class);
}
+ @Test
+ public void testDefaultAwsClientFactoryWithCredentialsProvider() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.AWS_REGION, Region.AWS_GLOBAL.toString());
+ properties.put(
+ AwsProperties.S3FILEIO_CREDENTIALS_PROVIDER,
+ SystemPropertyCredentialsProvider.class.getName());
+
+ AwsClientFactory defaultAwsClientFactory =
AwsClientFactories.from(properties);
+ Assertions.assertThat(defaultAwsClientFactory)
+ .isInstanceOf(AwsClientFactories.DefaultAwsClientFactory.class);
+
+ try (S3Client s3Client = defaultAwsClientFactory.s3()) {
+ Assertions.assertThat(s3Client != null);
+ URL url =
+ s3Client
+ .utilities()
+
.getUrl(GetUrlRequest.builder().bucket("test-bucket").key("test-key").build());
+ Assert.assertTrue(
+ url != null &&
url.toString().equals("https://test-bucket.s3.amazonaws.com/test-key";));
+ }
+ }
+
+ @Test
+ public void testDefaultAwsClientFactoryWithInvalidCredentialsProvider() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.AWS_REGION, Region.AWS_GLOBAL.toString());
+ properties.put(AwsProperties.S3FILEIO_CREDENTIALS_PROVIDER,
"invalidClassName");
+ AssertHelpers.assertThrows(
+ "Should fail if invalid credentials provider set",
+ IllegalArgumentException.class,
+ "The creation of an instance of invalidClassName failed",
+ () -> AwsClientFactories.from(properties).s3().close());
Review Comment:
[suggestion] Since there is another PR (#6977) planning on deprecating
`AssertHelpers`, shall we use `Assertions.assertThatThrownBy` here instead?
It may be something like:
```java
Assertions.assertThatThrownBy(() ->
AwsClientFactories.from(properties).s3().close())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("The creation of an instance of invalidClassName
failed");
```
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -91,13 +92,16 @@ private static AwsClientFactory loadClientFactory(String
impl, Map<String, Strin
static class DefaultAwsClientFactory implements AwsClientFactory {
private AwsProperties awsProperties;
+ private Region region;
+
DefaultAwsClientFactory() {
awsProperties = new AwsProperties();
}
@Override
public S3Client s3() {
return S3Client.builder()
+ .region(this.region)
Review Comment:
[Suggestion] Shall we follow the `applyMutation` pattern here
```java
.applyMutation(awsProperties:applyAwsRegionConfiguration)
```
and have the following method in `AwsProperties.java`
```java
public <T extends AwsSyncClientBuilder> void applyAwsRegionConfiguration(T
builder) {
if (awsRegion != null) {
builder.region(Region.of(awsRegion))
}
}
```
In this way, we do not need to do any further initialization in the
DefaultAwsClientFactories
What do you think about this change?
--
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]
