Copilot commented on code in PR #9410:
URL: https://github.com/apache/gravitino/pull/9410#discussion_r2598493968
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -174,6 +174,11 @@ public Map<String, String> getCatalogConfigToClient() {
return catalogConfigToClients;
}
+ @Override
+ protected boolean useDifferentClassLoader() {
+ return false;
+ }
Review Comment:
The new `useDifferentClassLoader()` override lacks test coverage. Given that
this method controls critical behavior related to JDBC driver cleanup and
prevents connection failures after idle time (as indicated by the PR title),
tests should verify:
1. That `useDifferentClassLoader()` returns `false` for
`CatalogWrapperForREST`
2. That closing a `CatalogWrapperForREST` instance does NOT deregister JDBC
drivers
3. That the catalog can be successfully reopened after being closed
(simulating the idle time scenario mentioned in the PR)
Consider adding a test case to `TestCatalogWrapperForREST` that validates
this behavior, especially for JDBC catalog backends.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -279,13 +279,27 @@ public boolean supportsViewOperations() {
@Override
public void close() throws Exception {
+ LOG.info("Closing IcebergCatalogWrapper for catalog: {}", catalog.name());
if (catalog instanceof AutoCloseable) {
// JdbcCatalog and ClosableHiveCatalog implement AutoCloseable and will
handle their own
// cleanup
((AutoCloseable) catalog).close();
}
metadataCache.close();
+ // For Iceberg REST server which use same classloader when recreating
catalog wrapper, the
Review Comment:
Typo in comment: "use" should be "uses" for proper grammar. The comment
should read "For Iceberg REST server which uses same classloader..."
```suggestion
// For Iceberg REST server which uses the same classloader when
recreating catalog wrapper, the
```
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -279,13 +279,27 @@ public boolean supportsViewOperations() {
@Override
public void close() throws Exception {
+ LOG.info("Closing IcebergCatalogWrapper for catalog: {}", catalog.name());
if (catalog instanceof AutoCloseable) {
// JdbcCatalog and ClosableHiveCatalog implement AutoCloseable and will
handle their own
// cleanup
((AutoCloseable) catalog).close();
}
metadataCache.close();
+ // For Iceberg REST server which use same classloader when recreating
catalog wrapper, the
+ // Driver couldn't be reloaded after deregister()
+ if (useDifferentClassLoader()) {
+ closeJdbcDriverResources();
+ }
+ }
+
+ // Whether to use same classloader when recreating IcebergCatalogWrapper
Review Comment:
The method `useDifferentClassLoader()` needs proper JavaDoc documentation.
Since this is a protected method that subclasses can override to control
important resource cleanup behavior, it should include:
- A description of what the method does
- When and why a subclass would override it
- The impact of returning true vs false
Example:
```java
/**
* Determines whether the catalog wrapper uses a different classloader when
recreating.
*
* <p>This controls whether JDBC driver resources (including MySQL's
AbandonedConnectionCleanupThread)
* should be cleaned up when the catalog is closed. Subclasses that reuse
the same classloader
* (like CatalogWrapperForREST) should override this to return false to
prevent driver reload issues.
*
* @return true if using different classloader (requires driver cleanup),
false otherwise
*/
```
```suggestion
/**
* Determines whether the catalog wrapper uses a different classloader
when recreating.
*
* <p>This controls whether JDBC driver resources (including MySQL's
AbandonedConnectionCleanupThread)
* should be cleaned up when the catalog is closed. Subclasses that reuse
the same classloader
* (such as for Iceberg REST server scenarios) should override this to
return {@code false}
* to prevent driver reload issues.
*
* <p>If {@code true}, JDBC driver resources will be cleaned up on close;
if {@code false},
* driver cleanup is skipped to avoid classloader issues.
*
* @return {@code true} if using a different classloader (requires driver
cleanup), {@code false} otherwise
*/
```
--
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]
