hudi-agent commented on code in PR #19778:
URL: https://github.com/apache/hudi/pull/19778#discussion_r3904790420
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieClient.java:
##########
@@ -142,19 +142,78 @@ private static TransactionManager
buildTransactionManager(HoodieEngineContext co
this.metrics = new HoodieMetrics(config, storage);
this.txnManager = transactionManager;
this.timeGenerator = timeGenerator;
- startEmbeddedServerView();
- runClientInitCallbacks();
+ try {
+ startEmbeddedServerView();
+ runClientInitCallbacks();
+ } catch (RuntimeException | Error e) {
+ // The constructor is not returning, so no caller ever gets an instance
to close(): release here or leak.
+ releaseAfterFailedInit(e);
Review Comment:
🤖 The guard here covers the base constructor's tail, so the
bad-callback-class case is handled — nice. But the client is always a subclass,
and the subclass work that runs after `super()` returns isn't covered:
`BaseHoodieWriteClient`'s `createIndex(writeConfig)` /
`emitIndexTypeMetrics(...)` and `SparkRDDWriteClient`'s `new
SparkRDDTableServiceClient(...)` all execute once the base constructor has
already started the embedded server, heartbeat, and txn-manager write lock. If
any of those throw, the same resources leak exactly as before, since no
instance escapes to call close(). Is closing that broader path in scope here,
or intended as a follow-up?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1661,12 +1661,35 @@ public void releaseResources(String instantTime) {
@Override
public void close() {
- // Stop timeline-server if running
- super.close();
- // Calling this here releases any resources used by your index, so make
sure to finish any related operations
- // before this point
- this.index.close();
- this.tableServiceClient.close();
+ // The index and the table service client are this class's own resources;
a failure while the
+ // base client releases its own must not leave them open.
+ Exception failure = null;
+ try {
+ // Stop timeline-server if running
+ super.close();
+ } catch (Exception e) {
+ failure = e;
+ }
+ try {
+ // Calling this here releases any resources used by your index, so make
sure to finish any related operations
+ // before this point
+ this.index.close();
+ } catch (Exception e) {
+ failure = failure == null ? e : addSuppressed(failure, e);
+ }
+ try {
+ this.tableServiceClient.close();
+ } catch (Exception e) {
+ failure = failure == null ? e : addSuppressed(failure, e);
+ }
+ if (failure != null) {
+ throw failure instanceof RuntimeException ? (RuntimeException) failure :
new HoodieException(failure);
+ }
+ }
Review Comment:
🤖 nit: the parent class calls this same helper `appendFailure` — could you
use the same name here for consistency? `addSuppressed` also shadows
`Throwable.addSuppressed`, which might cause a double-take. And since the
parent's version handles `null` internally, the subclass's callers wouldn't
need the inline `failure == null ? e :` guard if this were aligned.
<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]
