[GitHub] [iceberg] jackye1995 commented on a change in pull request #1618: refactor util to dynamically load location provider and file io

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

jackye1995 commented on a change in pull request #1618:
URL: https://github.com/apache/iceberg/pull/1618#discussion_r505980695



##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -174,7 +176,13 @@ public void commit(TableMetadata base, TableMetadata metadata) {
   @Override
   public FileIO io() {
     if (defaultFileIo == null) {
-      defaultFileIo = new HadoopFileIO(conf);
+      defaultFileIo = ClassLoaderUtil.fromProperty(
+          current() == null ? new HashMap<>() : current().properties(),
+          TableProperties.WRITE_FILE_IO_IMPL,
+          HadoopFileIO.class.getName(),
+          HadoopFileIO.class,
+          new Class<?>[] { Configuration.class },
+          new Object[]{ conf });

Review comment:
       The method supports no arg constructor, and also allows using multiple constructor options. The main reason for refactoring is because it has 2 lengthy try catch blocks for NoSuchMethodException when looking for a constructor, and also a ClassCastException when trying to cast the class.
   
   But I agree that it loses the ability to check default implementation at compile time. What if I leave the default class construction in the else blocks, and keep the util to only initialize the dynamic class?
   




----------------------------------------------------------------
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