rdblue commented on a change in pull request #2333:
URL: https://github.com/apache/iceberg/pull/2333#discussion_r600827578
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -214,24 +221,32 @@ public static FileIO loadFileIO(
Map<String, String> properties,
Configuration hadoopConf) {
LOG.info("Loading custom FileIO implementation: {}", impl);
- DynConstructors.Ctor<FileIO> ctor;
try {
- ctor = DynConstructors.builder(FileIO.class).impl(impl).buildChecked();
+ return loadFileIO(impl, hadoopConf, properties);
} catch (NoSuchMethodException e) {
throw new IllegalArgumentException(String.format(
- "Cannot initialize FileIO, missing no-arg constructor: %s", impl),
e);
+ "Cannot find a 0-arg or 1-arg constructor to initialize FileIO
implementation %s", impl), e);
+ } catch (ClassCastException e) {
+ throw new IllegalArgumentException(
+ String.format("Cannot initialize FileIO, %s does not implement
FileIO interface.", impl), e);
}
+ }
+ @SuppressWarnings("CatchBlockLogException")
+ private static FileIO loadFileIO(String impl, Configuration hadoopConf,
Map<String, String> properties)
+ throws NoSuchMethodException {
+ DynConstructors.Ctor<FileIO> ctor;
FileIO fileIO;
try {
+ ctor = DynConstructors.builder(FileIO.class).impl(impl).buildChecked();
fileIO = ctor.newInstance();
- } catch (ClassCastException e) {
- throw new IllegalArgumentException(
- String.format("Cannot initialize FileIO, %s does not implement
FileIO.", impl), e);
- }
-
- if (fileIO instanceof Configurable) {
- ((Configurable) fileIO).setConf(hadoopConf);
+ if (fileIO instanceof Configurable) {
+ ((Configurable) fileIO).setConf(hadoopConf);
+ }
+ } catch (NoSuchMethodException e) {
+ LOG.info("0-arg constructor not found for {}, will attempt to use Hadoop
Configuration constructor", impl);
+ ctor = DynConstructors.builder(Catalog.class).impl(impl,
Configuration.class).buildChecked();
Review comment:
I think this can be collapsed into a single DynConstructors call that
lists both implementations. The first one that is found will be selected. And
the `newInstance` method should discard arguments that are not needed so you
can use the same call for both.
--
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]
