jackye1995 commented on a change in pull request #1844:
URL: https://github.com/apache/iceberg/pull/1844#discussion_r533054971
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -171,45 +172,57 @@ public static Catalog loadCatalog(
/**
* Load a custom {@link FileIO} implementation.
+ */
+ public static FileIO loadFileIO(
+ String impl,
+ Map<String, String> properties,
+ Configuration hadoopConf) {
+ return loadCatalogConfigurable(impl, properties, hadoopConf, FileIO.class);
+ }
+
+ /**
+ * Load a custom implementation of a {@link CatalogConfigurable}.
* <p>
* The implementation must have a no-arg constructor.
- * If the class implements {@link Configurable},
+ * If the class implements Hadoop's {@link Configurable},
* a Hadoop config will be passed using {@link
Configurable#setConf(Configuration)}.
- * {@link FileIO#initialize(Map properties)} is called to complete the
initialization.
+ * {@link CatalogConfigurable#initialize(Map properties)} is called to
complete the initialization.
*
* @param impl full class name of a custom FileIO implementation
* @param hadoopConf hadoop configuration
+ * @param resultClass the final return class type
* @return FileIO class
* @throws IllegalArgumentException if class path not found or
* right constructor not found or
* the loaded class cannot be casted to the given interface type
*/
- public static FileIO loadFileIO(
+ public static <T extends CatalogConfigurable> T loadCatalogConfigurable(
String impl,
Map<String, String> properties,
- Configuration hadoopConf) {
- LOG.info("Loading custom FileIO implementation: {}", impl);
- DynConstructors.Ctor<FileIO> ctor;
+ Configuration hadoopConf,
+ Class<T> resultClass) {
+ LOG.info("Loading custom {} implementation: {}", resultClass.getName(),
impl);
+ DynConstructors.Ctor<T> ctor;
try {
- ctor = DynConstructors.builder(FileIO.class).impl(impl).buildChecked();
+ ctor = DynConstructors.builder(resultClass).impl(impl).buildChecked();
Review comment:
I don't think the user needs to reimplement the aws builders for every
service, see the example I provided in the conversation below. In the v2 AWS
client, the following aspects can be override:
1. credential provider
2. region
3. endpoint (url + port)
4. http client (for things like proxy)
5. client configuration
6. service configuration
we can either provide a way to dynamically configure each type of AWS
client, or dynamically configure each aspect of the clients. From my current
observation it is more common for users to want to use the same configuration
across all clients, but maybe also have some exceptions for some specific
clients. So it seems to make more sense to go with this proposed approach.
For your given example, yes information like port needs to be passed in, but
even in a normal application it needs to be passed in from some configuration
file. In this method you can also read your own configuration system instead of
reading from the catalog properties passed in.
You can read the example I gave below for more information, and there is no
need to initialize the client from the user.
----------------------------------------------------------------
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]
