rdblue commented on a change in pull request #1618:
URL: https://github.com/apache/iceberg/pull/1618#discussion_r505967826
##########
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:
I don't think this is actually cleaner than just using the reflection
utilities here. I understand wanting to reuse code, but I don't think that this
operation is a good candidate for it.
This mixes property checking with the reflect operation, and as a result the
default implementation is now loaded using reflection as well. That means that
we lose the compile time check that we're instantiating `HadoopFileIO`
correctly and lose the ability to easily refactor.
This also doesn't support more than one constructor. For `LocationProvider`,
the constructor could accept either location and properties or no args, but
this only checks for a single constructor and is harder to read as well.
The first thing I would change is to move the property check out of the
utility method and directly instantiate `HadoopFileIO` in an else block. After
making that change, there's little value in wrapping the reflection helpers.
The main complexity is catching `NoSuchMethodException` to return a better
error message (which is optional), but making that generic ends up being quite
complex:
```java
List<List<String>> allowedArgTypesString = allowedArgTypes.stream()
.map(argTypes ->
Arrays.stream(argTypes).map(Class::getName).collect(Collectors.toList()))
.collect(Collectors.toList());
throw new IllegalArgumentException(String.format(
"Unable to find a constructor for implementation %s of %s. " +
"Make sure the implementation is in classpath, and that it
either " +
"has a public no-arg constructor or one of the following
constructors: %s ",
impl, classInterface, allowedArgTypesString), e);
```
----------------------------------------------------------------
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]
