[GitHub] [iceberg] jackye1995 edited a comment on pull request #2333: Core: fix bug that HadoopFileIO cannot be dynamically loaded

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

jackye1995 edited a comment on pull request #2333:
URL: https://github.com/apache/iceberg/pull/2333#issuecomment-806164297


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


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