Core: fix bug that HadoopFileIO cannot be dynamically loaded #2333

jackye1995 · 2021-03-15T01:47:23Z

@aokolnychyi this should probably go to 0.11.1

HadoopFileIO currently does not have a no-arg constructor and could not be loaded by the dynamic FileIO loader. But our documentation claims it can be loaded, so we need to fix it asap. There are 2 ways to fix it,

add a no-arg constructor to HadoopFileIO, let it implement Configurable, and set configuration in setConf method.
(current approach in PR) allow the loader to load an implementation that has a constructor taking Hadoop configuration.

To do approach 1, we need to change HadoopFileIO class variable to be not final, and I am a bit hesitated to do that for performance concerns, so I went with approach 2 for now.

RussellSpitzer · 2021-03-15T14:54:47Z

core/src/main/java/org/apache/iceberg/CatalogUtil.java

    } catch (NoSuchMethodException e) {
-      throw new IllegalArgumentException(String.format(
-          "Cannot initialize FileIO, missing no-arg constructor: %s", impl), e);
+      LOG.error("No-arg constructor not found for {}, try to use Hadoop Configuration constructor", impl, e);


try -> will attempt to use

Just because the current wording makes it sound like the user needs to try to do something

Also shouldn't this be "info"? It's not necessarily an error right?

Thanks, updated

RussellSpitzer · 2021-03-15T14:57:38Z

core/src/main/java/org/apache/iceberg/CatalogUtil.java

+      LOG.error("No-arg constructor not found for {}, try to use Hadoop Configuration constructor", impl, e);
+      try {
+        ctor = DynConstructors.builder(Catalog.class).impl(impl, Configuration.class).buildChecked();
+        useNoArgConstructor = false;


Could we just invoke the constructor where we load it and skip the state variable passing through here?

Something like

try { ctor = .. fileIO = ctor.newInstance } catch { ctor = ... single are ctor fileIo = ctor.newInstance(hadoopConf) }

To avoid try nesting we could extract that to a helper function and then have something like

try { fileIO = loadFileIO(impl) } catch { Cannot find a 0-arg or 1-arg constructor to initalize FileImplemenation }

One additional question, do we always want the 0 arg constructor to get priority over a 1-arg? Or should we throw an error if you provide both?

do we always want the 0 arg constructor to get priority over a 1-arg

I think it is better to let 0 arg constructor to get priority, so it does not block any FileIO that might have both constructors.

To avoid try nesting we could extract that to a helper function

Yeah I tried about that but there was duplicated logic for finding ClassCastException. Using a helper function is probably a way to solve it, let me update with that, thank you!

jackye1995 · 2021-03-15T18:19:48Z

@RussellSpitzer updated based on all the comments, please take a look when you have time, thanks!

yyanyy · 2021-03-16T02:30:20Z

core/src/main/java/org/apache/iceberg/CatalogUtil.java

   * a Hadoop config will be passed using {@link Configurable#setConf(Configuration)}.
-   * {@link FileIO#initialize(Map properties)} is called to complete the initialization.
+   *
+   * For compatibility with HadoopFileIO,


I wonder if we starts to support 1-arg constructor as a valid case in the code and prints error messages about it, should we mention this as a fully supported case instead of a backward compatibility support? Especially that the reason for you to choose this option is performance concern.

Yes I also noticed this is a bit annoying to see a stack trace printed when trying to load it, let me just not print the error.

RussellSpitzer

Looks good to me

openinx · 2021-03-17T08:07:57Z

core/src/test/java/org/apache/iceberg/TestCatalogUtil.java

+    Configuration configuration = new Configuration();
+    configuration.set("key", "val");
+    FileIO fileIO = CatalogUtil.loadFileIO(
+        TestFileIOHadoopConstructor.class.getName(), Maps.newHashMap(), configuration);


Why not just load the HadoopFileIO directly ? Here we are trying to load an TestFileIOHadoopConstructor, I'm concerning that we could not detect the breaking case if someone change the HadoopFileIO constructor....

I don't think that would be possible, since HadoopFileIO is public and the constructor is also public, we cannot simply change it. But this is a good point, I can just load it instead of having this dummy test class.

rdblue · 2021-03-24T19:47:39Z

core/src/main/java/org/apache/iceberg/CatalogUtil.java

   * If the class implements {@link Configurable},
   * a Hadoop config will be passed using {@link Configurable#setConf(Configuration)}.
-   * {@link FileIO#initialize(Map properties)} is called to complete the initialization.
+   *


Paragraphs should be separated by <p>.

rdblue · 2021-03-24T19:49:45Z

core/src/main/java/org/apache/iceberg/CatalogUtil.java

+      }
+    } 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();


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.

rdblue · 2021-03-24T19:50:52Z

@jackye1995, can you elaborate on your performance concerns if the configuration isn't final? I'm not sure that would be a problem.

aokolnychyi · 2021-03-24T20:12:26Z

Sorry, I missed this PR. I can hold 0.11.1 RC for a little longer.

jackye1995 · 2021-03-24T20:26:39Z

@rdblue

I think this can be collapsed into a single DynConstructors call that lists both implementations.

The issue with that approach is that I still need a boolean to check if I need to call the setConf for no-arg constructor, and there was no way to get that boolean. I guess I can bypass that by checking instanceof Configurable for all FileIOs, so if we decide to go with that route I can make the change accordingly.

can you elaborate on your performance concerns if the configuration isn't final?

I would actually prefer to use this approach of adding no-arg consturctor for HadoopFileIO which makes the whole concept of FIleIO loading much simpler. But I see conf() is called in all read and write paths to create a serializable configuration to avoid Kryo serialization issue. There is definitely a small difference for using final versus not final, but the difference might not be significant from a broader view, I do not have a definitive answer for that.

jackye1995 · 2021-03-24T21:13:33Z

Sorry, I missed this PR. I can hold 0.11.1 RC for a little longer.

Thank you Anton!

rdblue · 2021-03-25T18:42:58Z

@jackye1995, let's go with the no-arg constructor and remove final. Configuration is primarily accessed to create FileIO, which can then have a final reference. So I don't think this is in a critical path and we shouldn't worry about it. Probably not worth making the contract for dynamic loading more complicated and difficult to maintain.

jackye1995 · 2021-03-25T19:13:46Z

@rdblue I was just going to reply, I did some benchmarking, as expected there is a difference but it was at microsecond level, so I think it should be fine to just remove the final. I think I was overly conservative on this, thanks for the suggestion!

aokolnychyi · 2021-03-29T21:56:45Z

Thanks everyone! I am going to include this change in 0.11.1.

github-actions bot added the core label Mar 15, 2021

RussellSpitzer reviewed Mar 15, 2021

View reviewed changes

yyanyy approved these changes Mar 16, 2021

View reviewed changes

RussellSpitzer approved these changes Mar 16, 2021

View reviewed changes

openinx reviewed Mar 17, 2021

View reviewed changes

rdblue reviewed Mar 24, 2021

View reviewed changes

Core: fix bug that HadoopFileIO cannot be dynamically loaded

494d4f9

rdblue approved these changes Mar 26, 2021

View reviewed changes

rdblue merged commit d30a225 into apache:master Mar 26, 2021

aokolnychyi pushed a commit that referenced this pull request Mar 29, 2021

Core: Support dynamic loading for HadoopFileIO (#2333)

1c007f2

aokolnychyi added this to the Java 0.11.1 Release milestone Mar 30, 2021

jackye1995 mentioned this pull request Apr 2, 2021

Core: Add SerializableTable #2403

Merged

coolderli pushed a commit to coolderli/iceberg that referenced this pull request Apr 26, 2021

Core: Support dynamic loading for HadoopFileIO (apache#2333)

8d43042

stevenzwu pushed a commit to stevenzwu/iceberg that referenced this pull request Jul 28, 2021

Core: Support dynamic loading for HadoopFileIO (apache#2333)

2cf437e

Core: fix bug that HadoopFileIO cannot be dynamically loaded #2333

Core: fix bug that HadoopFileIO cannot be dynamically loaded #2333

Uh oh!

Conversation

jackye1995 commented Mar 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Mar 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Mar 24, 2021

Uh oh!

aokolnychyi commented Mar 24, 2021

Uh oh!

jackye1995 commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Mar 24, 2021

Uh oh!

rdblue commented Mar 25, 2021

Uh oh!

jackye1995 commented Mar 25, 2021

Uh oh!

aokolnychyi commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RussellSpitzer Mar 15, 2021 •

edited

Loading

jackye1995 commented Mar 24, 2021 •

edited

Loading