szehon-ho commented on code in PR #6698:
URL: https://github.com/apache/iceberg/pull/6698#discussion_r1136285990
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -53,26 +67,27 @@ public class CachedClientPool implements
ClientPool<IMetaStoreClient, TException
properties,
CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS_DEFAULT);
+ this.key =
extractKey(properties.get(CatalogProperties.CLIENT_POOL_CACHE_KEYS), conf);
init();
}
@VisibleForTesting
HiveClientPool clientPool() {
- return clientPoolCache.get(metastoreUri, k -> new
HiveClientPool(clientPoolSize, conf));
+ return clientPoolCache.get(key, k -> new HiveClientPool(clientPoolSize,
conf));
}
private synchronized void init() {
if (clientPoolCache == null) {
clientPoolCache =
Caffeine.newBuilder()
.expireAfterAccess(evictionInterval, TimeUnit.MILLISECONDS)
- .removalListener((key, value, cause) -> ((HiveClientPool)
value).close())
+ .removalListener((ignored, value, cause) -> ((HiveClientPool)
value).close())
Review Comment:
Nit: I think we don't need this change particularly and can revert.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -87,4 +102,89 @@ public <R> R run(Action<R, IMetaStoreClient, TException>
action, boolean retry)
throws TException, InterruptedException {
return clientPool().run(action, retry);
}
+
+ @VisibleForTesting
+ static Key extractKey(String cacheKeys, Configuration conf) {
+ // generate key elements in a certain order, so that the Key instances are
comparable
+ List<Object> elements = Lists.newArrayList();
+ elements.add(conf.get(HiveConf.ConfVars.METASTOREURIS.varname, ""));
+ if (cacheKeys == null || cacheKeys.isEmpty()) {
+ return Key.of(elements);
+ }
+
+ Set<KeyElementType> types =
Sets.newTreeSet(Comparator.comparingInt(Enum::ordinal));
+ Map<String, String> confElements = Maps.newTreeMap();
+ for (String element : cacheKeys.split(",", -1)) {
+ String trimmed = element.trim();
+ if (trimmed.toLowerCase(Locale.ROOT).startsWith(CONF_ELEMENT_PREFIX)) {
+ String key = trimmed.substring(CONF_ELEMENT_PREFIX.length());
+ ValidationException.check(
+ !confElements.containsKey(key), "Conf key element %s already
specified", key);
+ confElements.put(key, conf.get(key));
+ } else {
+ KeyElementType type = KeyElementType.valueOf(trimmed.toUpperCase());
+ switch (type) {
+ case UGI:
+ case USER_NAME:
+ ValidationException.check(
Review Comment:
Nit: prefer to split into two line (ValidationCheck and then add), as its
more readable.
Took me awhile to notice the ValidationCheck itself is side-effecting.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -87,4 +102,89 @@ public <R> R run(Action<R, IMetaStoreClient, TException>
action, boolean retry)
throws TException, InterruptedException {
return clientPool().run(action, retry);
}
+
+ @VisibleForTesting
+ static Key extractKey(String cacheKeys, Configuration conf) {
+ // generate key elements in a certain order, so that the Key instances are
comparable
+ List<Object> elements = Lists.newArrayList();
+ elements.add(conf.get(HiveConf.ConfVars.METASTOREURIS.varname, ""));
+ if (cacheKeys == null || cacheKeys.isEmpty()) {
Review Comment:
Selfishly, I would add 'default.catalog' to here as well, as I'm not exactly
sure any use case where Iceberg re-uses HMS Client with different catalogs.
(As we never allow user to pass in catalog explicitly to HMSClient). But I'm
ok to do it in another pr which we can contribute, for readability.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java:
##########
@@ -87,4 +102,89 @@ public <R> R run(Action<R, IMetaStoreClient, TException>
action, boolean retry)
throws TException, InterruptedException {
return clientPool().run(action, retry);
}
+
+ @VisibleForTesting
+ static Key extractKey(String cacheKeys, Configuration conf) {
+ // generate key elements in a certain order, so that the Key instances are
comparable
+ List<Object> elements = Lists.newArrayList();
+ elements.add(conf.get(HiveConf.ConfVars.METASTOREURIS.varname, ""));
+ if (cacheKeys == null || cacheKeys.isEmpty()) {
+ return Key.of(elements);
+ }
+
+ Set<KeyElementType> types =
Sets.newTreeSet(Comparator.comparingInt(Enum::ordinal));
+ Map<String, String> confElements = Maps.newTreeMap();
+ for (String element : cacheKeys.split(",", -1)) {
+ String trimmed = element.trim();
+ if (trimmed.toLowerCase(Locale.ROOT).startsWith(CONF_ELEMENT_PREFIX)) {
+ String key = trimmed.substring(CONF_ELEMENT_PREFIX.length());
+ ValidationException.check(
+ !confElements.containsKey(key), "Conf key element %s already
specified", key);
+ confElements.put(key, conf.get(key));
Review Comment:
Question, how we are sorting conf elements?
--
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
