[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6698: Core, Hive: Support pluggable ClientPool

["szehon-ho (via GitHub)" <gi...@apache.org>]

szehon-ho commented on code in PR #6698:
URL: https://github.com/apache/iceberg/pull/6698#discussion_r1092703260


##########
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 think the context is from https://github.com/apache/iceberg/pull/6175 (more issues scattered).  The general problem being the CachedClientPool that's always on in Iceberg Hive, which is a global cache that doesnt work in all environments, because the current key (metastoreUri) collides too much, leading to the wrong client used for a lot of use cases.
   
   It seems simpler to me to just make cache pluggable, but I am not sure if there's a better solution I'm not seeing to make it non-dynamic?  If we dont want user to turn off CachedClientPool, at least we need to have a pluggable conf => key generator?  Or maybe just allow them to use the toggle between CachedClientPool and the raw ClientPool as an option (as a way to turn off the global cache)?  cc @pvary @flyrain  as well if any thoughts



-- 
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-unsubscribe@iceberg.apache.org

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