Copilot commented on code in PR #13450:
URL: https://github.com/apache/gravitino/pull/13450#discussion_r4073237961
##########
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:
Using `dataSource instanceof AutoCloseable` as a proxy for ownership is not
reliable: many externally-managed/injected pools (e.g., HikariDataSource) are
also `AutoCloseable` and would now be closed unexpectedly by the storage. To
match the PR description (“close it owns”), introduce an explicit ownership
signal (e.g., a `boolean closeDataSourceOnClose` field set by the factory, or a
dedicated wrapper type) so injected `AutoCloseable` sources can remain
externally managed.
##########
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:
`close()` is currently not guaranteed to be idempotent: repeated calls will
invoke `AutoCloseable.close()` multiple times, which can throw for some
implementations. Consider guarding with a `closed` flag (e.g., `AtomicBoolean`)
and/or nulling out the closeable reference after successful close, so repeated
`close()` calls are safe and do not generate spurious IOExceptions.
##########
core/src/test/java/org/apache/gravitino/stats/storage/TestJdbcPartitionStatisticStorage.java:
##########
@@ -101,6 +102,21 @@ public void setUp() throws Exception {
storage = new JdbcPartitionStatisticStorage(mockDataSource);
}
+ @Test
+ public void testCloseClosesOwnedDataSource() throws Exception {
+ // The storage is the only reachable owner of the pooled DataSource (the
+ // factory is discarded by the manager), so close() must release it.
+ BasicDataSource dataSource = new BasicDataSource();
+ dataSource.setDriverClassName("org.h2.Driver");
+ dataSource.setUrl("jdbc:h2:mem:stats_close_test;DB_CLOSE_DELAY=-1");
+
+ try (JdbcPartitionStatisticStorage owned = new
JdbcPartitionStatisticStorage(dataSource)) {
+ // no-op: never open a connection
+ }
+
+ assertTrue(dataSource.isClosed());
+ }
Review Comment:
This test covers a single close path, but it doesn’t assert the idempotency
described in the PR text (calling `close()` twice should not throw and should
keep the pool closed). Consider extending the test to call `owned.close()`
explicitly after the try-with-resources (or use two explicit calls) and assert
no exception plus `isClosed()`. If you implement explicit ownership
(recommended), add a test case ensuring an injected/external `AutoCloseable`
DataSource is NOT closed when the storage does not own it.
--
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]
