hudi-agent commented on code in PR #20219:
URL: https://github.com/apache/hudi/pull/20219#discussion_r4235297810
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/HiveQueryDDLExecutor.java:
##########
@@ -65,18 +65,23 @@ public class HiveQueryDDLExecutor extends
QueryBasedDDLExecutor {
// Owned by HoodieHiveSyncClient; close() is delegated through there. See
// HiveMetaStoreClientPool javadoc for the usage contract (partition-row ops
only).
private final Option<HiveMetaStoreClientPool> metaStoreClientPool;
+ // Run on the calling thread before the session's Driver runs statements:
its first statement
+ // sets up the session's authorization, which replaces the Hive instance
cached for the thread.
+ private final Runnable beforeSessionStatements;
public HiveQueryDDLExecutor(HiveSyncConfig config, IMetaStoreClient
metaStoreClient) {
- this(config, metaStoreClient, Option.empty(), Option.empty());
+ this(config, metaStoreClient, Option.empty(), Option.empty(), () -> { });
}
Review Comment:
🤖 nit: could the no-arg-callback constructor use a named no-op (or the 2-arg
constructor pass `null`-safe `Option`) instead of `() -> { }`, so the intent of
"no hook" is explicit?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -167,10 +170,44 @@ public HoodieHiveSyncClient(HiveSyncConfig config,
HoodieTableMetaClient metaCli
// such window; both close() methods are idempotent, so overlapping with
the
// executor's cleanup (or buildHiveQueryDDLExecutor's rollback) is
harmless.
closePartitionPoolsQuietly();
+ closeClient();
throw new HoodieHiveSyncException("Failed to create
HiveMetaStoreClient", e);
}
}
+ /**
+ * Takes the instance that owns {@link #client} out of Hive's cache for this
thread, if it is
+ * there. Hive replaces the cached instance, closing its client, when a Hive
session on the thread
+ * brings a different metastore conf, as a HiveQL-mode Driver's session does
on its first
+ * statement, by setting the metastore filter hook for authorization. Until
then the client stays
+ * cached, so that the session's teardown reuses it rather than opening a
connection of its own.
+ */
+ private void releaseClientFromHiveCache() {
+ if (clientCachedOnThread == Thread.currentThread()) {
+ Hive.set(null);
+ clientCachedOnThread = null;
+ }
+ }
+
+ /**
+ * Closes {@link #client}, which only closing it directly releases once it
is out of Hive's cache,
+ * including an underlying client that RetryingMetaStoreClient rebuilt after
a transient
+ * TException. Hive.closeCurrent() then closes whatever instance is left in
the cache.
+ */
+ private void closeClient() {
+ if (client == null) {
+ return;
Review Comment:
🤖 nit: could `closeClient()` be renamed to something like
`closeClientAndHiveCache()`? It also calls `Hive.closeCurrent()` and resets the
cached-thread marker, which the name doesn't suggest.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
