[GitHub] [iceberg] yyanyy commented on a change in pull request #1844: AWS: support custom credentials, region and assume role functionality

[GitBox <gi...@apache.org>]

yyanyy commented on a change in pull request #1844:
URL: https://github.com/apache/iceberg/pull/1844#discussion_r533863696



##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -37,12 +37,16 @@
  * See {@link S3URI#VALID_SCHEMES} for the list of supported S3 URI schemes.
  */
 public class S3FileIO implements FileIO {
-  private final SerializableSupplier<S3Client> s3;
+  private SerializableSupplier<S3Client> s3;
   private AwsProperties awsProperties;
   private transient S3Client client;
 
+  /**
+   * No-arg constructor to load the FileIO dynamically.
+   * <p>
+   * All fields are initialized by calling {@link S3FileIO#initialize(Map)} later.
+   */
   public S3FileIO() {
-    this(AwsClientUtil::defaultS3Client);
   }
 
   public S3FileIO(SerializableSupplier<S3Client> s3) {

Review comment:
       I think `s3` passed in from this and the constructor below will be overwritten in `initialize`, do we want to consolidate them/mention in javadoc? 

##########
File path: api/src/main/java/org/apache/iceberg/io/FileIO.java
##########
@@ -20,7 +20,7 @@
 package org.apache.iceberg.io;
 
 import java.io.Serializable;
-import java.util.Map;
+import org.apache.iceberg.catalog.CatalogConfigurable;

Review comment:
       I remember reading about iceberg being able to support catalog backed by multiple cloud vendors based on its design, wondering if we do want to support that (and thus might want to change the name `CatalogConfigurable`).

##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -118,7 +118,7 @@ public void loadCustomFileIO_badArg() {
   public void loadCustomFileIO_badClass() {
     AssertHelpers.assertThrows("cannot cast",
         IllegalArgumentException.class,
-        "does not implement FileIO",
+        "does not implement org.apache.iceberg.io.FileIO",

Review comment:
       I think `class.getSimpleName()` could avoid changing this, but leaving as is doesn't harm. 




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org