[GitHub] [iceberg] pvary commented on a diff in pull request #5378: Hive: More distinctive cached client pool key to avoid conflict

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

pvary commented on code in PR #5378:
URL: https://github.com/apache/iceberg/pull/5378#discussion_r933632062


##########
hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -56,9 +57,16 @@ public class CachedClientPool implements ClientPool<IMetaStoreClient, TException
     init();
   }
 
+  @VisibleForTesting
+  static String cacheKey(Configuration conf) {
+    return String.format(
+        "%s:%s",
+        conf.get(HiveConf.ConfVars.METASTOREURIS.varname, ""), conf.get(CATALOG_DEFAULT, ""));

Review Comment:
   If I understand correctly Spark sends every configuration value to the HMS Client which starts with `spark.sql.catalog.catalog1.hadoop.` in this case.
   Choosing `metastore.catalog.default` as the only thing used in the cacheKey seems like an ad-hoc decision to me. Some interesting configs:
   - CLIENT_SOCKET_TIMEOUT
   - HMS_HANDLER_ATTEMPTS
   - HMS_HANDLER_INTERVAL
   Different users might require different values here...
   
   Would it be "overengineering" to have a static list of config keys to use in the `cacheKey`, and use a hash, or a join of the values for the cache key?



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