danny0405 commented on code in PR #20219:
URL: https://github.com/apache/hudi/pull/20219#discussion_r4213790174
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -235,7 +235,14 @@ private IMetaStoreClient
createMetaStoreClient(HiveSyncConfig config) {
new Class<?>[] {HiveSyncConfig.class},
config);
}
- return IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ IMetaStoreClient metaStoreClient =
IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ // Hive caches the instance that owns this client for the thread, and
replaces it, closing
+ // this client, whenever a Hive session on the thread brings a different
metastore conf. A
+ // HiveQL-mode Driver's session does, by setting the metastore filter
hook for authorization.
+ // Taken out of the cache, the client is left to this class, which
closes it in close().
+ // Hive.closeCurrent() would close the client too, hence the set.
+ Hive.set(null);
Review Comment:
**[P2] Close the detached client if construction fails.** After this removes
the client from Hive's cache, a subsequent failure in `setMetaConf()` or DDL
executor initialization leaves it without a cleanup path: the
`HoodieHiveSyncClient` constructor's catch block only calls
`closePartitionPoolsQuietly()`, and the caller never receives the constructed
object to close. Even `Hive.closeCurrent()` can no longer release it. In a
focused parent-versus-head probe injecting a `setMetaConf()` failure, the
mocked client's close count after `Hive.closeCurrent()` changes from 1 to 0.
Repeated initialization failures can therefore accumulate metastore connections
instead of reusing or cleaning up the cached client. Please explicitly close
`this.client` in the constructor's failure path and add a regression test for
failure after acquisition.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -235,7 +235,14 @@ private IMetaStoreClient
createMetaStoreClient(HiveSyncConfig config) {
new Class<?>[] {HiveSyncConfig.class},
config);
}
- return IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ IMetaStoreClient metaStoreClient =
IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ // Hive caches the instance that owns this client for the thread, and
replaces it, closing
+ // this client, whenever a Hive session on the thread brings a different
metastore conf. A
+ // HiveQL-mode Driver's session does, by setting the metastore filter
hook for authorization.
+ // Taken out of the cache, the client is left to this class, which
closes it in close().
+ // Hive.closeCurrent() would close the client too, hence the set.
+ Hive.set(null);
+ return metaStoreClient;
Review Comment:
**[P2] Avoid opening an additional connection for no-op HiveQL sync
cleanup.** Unconditionally detaching the client also removes reuse where the
session's metastore configuration has not changed. In particular,
`HiveSyncTool` can return early for an already-synced table without running
DDL, but `HiveQueryDDLExecutor.close()` still calls `SessionState.close()`.
Hive 2.3.10's `unCacheDataNucleusClassLoaders()` then calls
`Hive.get(sessionConf).getMSC()`: with the cache cleared, cleanup creates
another metastore client instead of reusing the existing one. A focused probe
using the real cleanup helper and mocked client construction changes from 1
client on the parent to 2 on this head; this is connection-count evidence, not
a latency benchmark. For frequent no-op syncs, an extra remote
connection/authentication handshake per cycle could be a meaningful regression,
with no forced reconnect being saved on that path. Please cover the
already-synced/no-DDL path with a connection-count tes
t and avoid the cleanup-only connection; also measure repeated no-op sync
latency before accepting this tradeoff.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
