szehon-ho commented on code in PR #6698:
URL: https://github.com/apache/iceberg/pull/6698#discussion_r1091488029
##########
core/src/main/java/org/apache/iceberg/CatalogProperties.java:
##########
@@ -119,6 +119,8 @@ private CatalogProperties() {}
"client.pool.cache.eviction-interval-ms";
public static final long CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS_DEFAULT =
TimeUnit.MINUTES.toMillis(5);
+ /** Name of the custom {@link ClientPool} implementation class. */
+ public static final String CLIENT_POOL_IMPL = "client.pool.impl";
Review Comment:
I actually like dot better, but maybe we should make it with a hyphen like
the other impl properties?
##########
core/src/main/java/org/apache/iceberg/ClientPool.java:
##########
@@ -26,4 +28,16 @@
<R> R run(Action<R, C, E> action) throws E, InterruptedException;
<R> R run(Action<R, C, E> action, boolean retry) throws E,
InterruptedException;
+
+ /**
+ * Initialize the client pool with catalog properties and an optional hadoop
configuration.
Review Comment:
nit: do we need 'optional' to describe hadoop configuration? (its always
passed in, so seems redundant to say this)
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -439,4 +439,40 @@ public static MetricsReporter loadMetricsReporter(String
impl) {
return reporter;
}
+
+ /**
+ * Load a custom ClientPool implementation.
+ *
+ * <p>The ClientPool must have a no-arg constructor. {@link
ClientPool#initialize(Map, Object)} is
+ * called to complete the initialization.
+ *
+ * @param impl ClientPool implementation full class name
+ * @param properties catalog properties
+ * @param conf hadoop configuration if needed
+ * @return initialized ClientPool object
+ * @throws IllegalArgumentException if no-arg constructor not found or error
during initialization
+ */
+ public static <C, E extends Exception> ClientPool<C, E> loadClientPool(
+ String impl, Map<String, String> properties, Object conf) {
+ Preconditions.checkNotNull(
+ impl, "Cannot initialize custom ClientPool, impl class name is null");
+ DynConstructors.Ctor<ClientPool<C, E>> ctor;
+ try {
+ ctor =
DynConstructors.builder(ClientPool.class).impl(impl).buildChecked();
+ } catch (NoSuchMethodException e) {
+ throw new IllegalArgumentException(
+ String.format("Cannot initialize ClientPool implementation %s: %s",
impl, e.getMessage()),
Review Comment:
nit: not sure we need to put e.message again, if we already include e?
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -439,4 +439,40 @@ public static MetricsReporter loadMetricsReporter(String
impl) {
return reporter;
}
+
+ /**
+ * Load a custom ClientPool implementation.
+ *
+ * <p>The ClientPool must have a no-arg constructor. {@link
ClientPool#initialize(Map, Object)} is
+ * called to complete the initialization.
+ *
+ * @param impl ClientPool implementation full class name
+ * @param properties catalog properties
+ * @param conf hadoop configuration if needed
+ * @return initialized ClientPool object
+ * @throws IllegalArgumentException if no-arg constructor not found or error
during initialization
+ */
+ public static <C, E extends Exception> ClientPool<C, E> loadClientPool(
+ String impl, Map<String, String> properties, Object conf) {
+ Preconditions.checkNotNull(
Review Comment:
Can we add Log.info here, like "Loading custom client pool implementation:
%s"
##########
core/src/main/java/org/apache/iceberg/ClientPool.java:
##########
@@ -26,4 +28,16 @@
<R> R run(Action<R, C, E> action) throws E, InterruptedException;
<R> R run(Action<R, C, E> action, boolean retry) throws E,
InterruptedException;
+
+ /**
+ * Initialize the client pool with catalog properties and an optional hadoop
configuration.
+ *
+ * <p>A custom ClientPool implementation must have a no-arg constructor. A
Catalog using the
+ * ClientPool will first use this constructor to create an instance of the
pool, and then call
+ * this method to initialize the pool.
+ *
+ * @param properties catalog properties
+ * @param hadoopConf hadoop configuration
+ */
+ default void initialize(Map<String, String> properties, Object hadoopConf) {}
Review Comment:
Actually did you consider following FileIO way and only passing it in, if
the impl implements Configurable? Maybe we can avoid the ugly Object here.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -107,7 +107,13 @@ public void initialize(String inputName, Map<String,
String> properties) {
? new HadoopFileIO(conf)
: CatalogUtil.loadFileIO(fileIOImpl, properties, conf);
- this.clients = new CachedClientPool(conf, properties);
+ if (properties.containsKey(CatalogProperties.CLIENT_POOL_IMPL)) {
Review Comment:
Nit: not that I have an opinion which style is better, but can we make this
block more closely follow the previous block in terms of style (using the
question-mark colon operator). So code is more consistent to understand at a
glance.
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -439,4 +439,40 @@ public static MetricsReporter loadMetricsReporter(String
impl) {
return reporter;
}
+
+ /**
+ * Load a custom ClientPool implementation.
+ *
+ * <p>The ClientPool must have a no-arg constructor. {@link
ClientPool#initialize(Map, Object)} is
+ * called to complete the initialization.
+ *
+ * @param impl ClientPool implementation full class name
+ * @param properties catalog properties
+ * @param conf hadoop configuration if needed
+ * @return initialized ClientPool object
+ * @throws IllegalArgumentException if no-arg constructor not found or error
during initialization
+ */
+ public static <C, E extends Exception> ClientPool<C, E> loadClientPool(
+ String impl, Map<String, String> properties, Object conf) {
+ Preconditions.checkNotNull(
+ impl, "Cannot initialize custom ClientPool, impl class name is null");
Review Comment:
nit, do you think we can also have a better word in the error message than
'impl', ie use the actual property constant name, and we can pass it to
Preconditions via the .checkNonNull(errorMsgTemplate, errorMsgArgs)?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
