hudi-agent commented on code in PR #19718:
URL: https://github.com/apache/hudi/pull/19718#discussion_r3846014195
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/HiveQueryDDLExecutor.java:
##########
@@ -315,6 +316,10 @@ public void close() {
}
if (hiveDriver != null) {
Review Comment:
🤖 Since the whole point here is to guarantee the shutdown hook gets removed,
I wonder if `destroy()` should run even when `close()` throws. As written, if
`hiveDriver.close()` throws a runtime exception, `destroy()` is skipped and the
hook leak persists (plus the exception propagates out of `close()`). Would a
try/finally around the two calls make the hook removal robust?
<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/util/HiveDriverPool.java:
##########
@@ -207,6 +207,11 @@ private void tearDown() {
if (worker.driver != null) {
try {
worker.driver.close();
Review Comment:
🤖 Same close-then-destroy ordering question as in HiveQueryDDLExecutor:
`close()` and `destroy()` share one try/catch, so if `worker.driver.close()`
throws it's logged but `destroy()` is skipped, leaving that worker's Driver in
the hook set. Might be worth giving `destroy()` its own try (or a finally) so a
close() failure doesn't defeat the hook removal for that worker.
<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]
