Copilot commented on code in PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798#discussion_r3679589827
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -459,9 +459,11 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk,
EventLoopGroup eventLo
log.error()
.exception(ce)
.log("Failed to initialize metadata client driver using
invalid metadata service uri");
+ close();
throw new IOException("Failed to initialize metadata client
driver", ce);
Review Comment:
Calling close() inside this catch can throw InterruptedException (and the
method signature also allows BKException), which would mask the original
ConfigurationException and change what build()/ctor reports to callers. It’s
better to preserve the original failure and treat close() failures as
suppressed while restoring the interrupt flag.
This issue also appears on line 464 of the same file.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -537,6 +538,7 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk,
EventLoopGroup eventLo
this.ledgerManagerFactory =
this.metadataDriver.getLedgerManagerFactory();
} catch (MetadataException e) {
+ close();
throw new IOException("Failed to initialize ledger manager
factory", e);
}
Review Comment:
This adds cleanup for getLedgerManagerFactory() failures, but other
constructor failure points can still leak previously-created resources because
they aren’t wrapped (e.g. initializeEnsemblePlacementPolicy(...) is declared
throws IOException at BookKeeper.java:579-592, and BookieClientImpl’s ctor
throws IOException). Consider wrapping the whole constructor body in a
try/finally with an "initialized" flag and invoking close() in finally when
initialization doesn’t complete, rather than sprinkling close() calls in a few
catches.
--
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]
