LuciferYang commented on code in PR #13450:
URL: https://github.com/apache/gravitino/pull/13450#discussion_r4081855488
##########
core/src/main/java/org/apache/gravitino/stats/storage/JdbcPartitionStatisticStorage.java:
##########
@@ -356,8 +356,18 @@ public void updateStatistics(
@Override
public void close() throws IOException {
- // DataSource lifecycle is managed externally by the factory
LOG.debug("Closing JdbcPartitionStatisticStorage");
+ // This storage is the only reachable owner of the pooled DataSource: the
+ // factory that created it is discarded by the manager, so close must
+ // release the pool. DataSources that do not implement AutoCloseable keep
+ // their externally-managed lifecycle.
+ if (dataSource instanceof AutoCloseable) {
+ try {
+ ((AutoCloseable) dataSource).close();
+ } catch (Exception e) {
+ throw new IOException("Failed to close JDBC DataSource", e);
+ }
+ }
Review Comment:
In production the storage is the sole owner: the factory creates a
`BasicDataSource`, hands it to the storage, and is then discarded, so there is
no externally-managed pool in play. The `instanceof AutoCloseable` guard is
there to skip the non-closeable mock `DataSource` the tests inject. An
externally-owned `AutoCloseable` pool (e.g. a caller-managed Hikari) would only
reach here through the test constructor, which no production path uses. I kept
the guard instead of an explicit `ownsDataSource` flag to avoid widening the
change, but I'm happy to make ownership explicit if you'd prefer.
--
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]
