This is an automated email from the ASF dual-hosted git repository. github-bot pushed a commit to branch cherry-pick-89216725-to-branch-1.2 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 293cad3989e156638ea739eabe270cd73cb53eda Author: Qi Yu <[email protected]> AuthorDate: Mon Mar 9 18:07:52 2026 +0800 [#10253] Optimize driver degisteration logic for JDBC catalog to avoid possible OOM problem (#10255) ### What changes were proposed in this pull request? This pull request refactors the JDBC driver deregistration logic across multiple catalog implementations to centralize and standardize the process. The main change is moving driver deregistration from individual catalog subclasses to the base `JdbcCatalogOperations` class, reducing code duplication and improving maintainability. Additionally, the pull request updates related tests and adapts the OceanBase catalog to use a more appropriate operations class. ### Why are the changes needed? To avoid resource leakage Fix: #10253 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? UTs --------- Co-authored-by: Copilot <[email protected]> # Conflicts: # catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/HologresCatalogOperations.java --- .../operations/ClickHouseCatalogOperations.java | 15 +---- .../hologres/HologresCatalogOperations.java | 25 ++++--- .../catalog/oceanbase/OceanBaseCatalog.java | 4 +- .../catalog/jdbc/JdbcCatalogOperations.java | 24 +++++++ .../MySQLProtocolCompatibleCatalogOperations.java | 13 ++-- .../catalog/jdbc/TestJdbcCatalogOperations.java | 78 ++++++++++++++++++++++ .../postgresql/PostgreSQLCatalogOperations.java | 13 +--- 7 files changed, 129 insertions(+), 43 deletions(-) diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseCatalogOperations.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseCatalogOperations.java index 01631f3337..fb9a3d7140 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseCatalogOperations.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseCatalogOperations.java @@ -21,6 +21,7 @@ package org.apache.gravitino.catalog.clickhouse.operations; import java.sql.Driver; import java.sql.DriverManager; +import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.JdbcCatalogOperations; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; @@ -45,17 +46,7 @@ public class ClickHouseCatalogOperations extends JdbcCatalogOperations { } @Override - public void close() { - super.close(); - - // deregister ClickHouse JDBC driver to prevent memory leak in long-running applications - try { - Driver clickhouseDriver = - DriverManager.getDriver("jdbc:clickhouse://dummy_address:8443/default"); - deregisterDriver(clickhouseDriver); - } catch (java.sql.SQLException e) { - // log and ignore - System.err.println("Failed to deregister ClickHouse JDBC driver: " + e.getMessage()); - } + protected Driver getDriver() throws SQLException { + return DriverManager.getDriver("jdbc:clickhouse://dummy_address:8443/default"); } } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java b/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/HologresCatalogOperations.java similarity index 76% copy from catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java copy to catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/HologresCatalogOperations.java index 527a27072a..2c2442569e 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java +++ b/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/HologresCatalogOperations.java @@ -16,10 +16,11 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.gravitino.catalog.postgresql; +package org.apache.gravitino.catalog.hologres; import java.sql.Driver; import java.sql.DriverManager; +import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.JdbcCatalogOperations; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; @@ -27,9 +28,15 @@ import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations; -public class PostgreSQLCatalogOperations extends JdbcCatalogOperations { +/** + * Hologres catalog operations implementation. + * + * <p>Since Hologres uses the PostgreSQL JDBC driver, this class handles the driver deregistration + * properly to avoid memory leaks when the catalog is closed. + */ +public class HologresCatalogOperations extends JdbcCatalogOperations { - public PostgreSQLCatalogOperations( + public HologresCatalogOperations( JdbcExceptionConverter exceptionConverter, JdbcTypeConverter jdbcTypeConverter, JdbcDatabaseOperations databaseOperation, @@ -44,15 +51,7 @@ public class PostgreSQLCatalogOperations extends JdbcCatalogOperations { } @Override - public void close() { - super.close(); - try { - // Unload the PostgreSQL driver, only Unload the driver if it is loaded by - // IsolatedClassLoader. - Driver pgDriver = DriverManager.getDriver("jdbc:postgresql://dummy_address:12345/"); - deregisterDriver(pgDriver); - } catch (Exception e) { - LOG.warn("Failed to deregister PostgreSQL driver", e); - } + protected Driver getDriver() throws SQLException { + return DriverManager.getDriver("jdbc:postgresql://dummy_address:12345/"); } } diff --git a/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java b/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java index 1231926441..200d501de5 100644 --- a/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java +++ b/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java @@ -20,7 +20,7 @@ package org.apache.gravitino.catalog.oceanbase; import java.util.Map; import org.apache.gravitino.catalog.jdbc.JdbcCatalog; -import org.apache.gravitino.catalog.jdbc.JdbcCatalogOperations; +import org.apache.gravitino.catalog.jdbc.MySQLProtocolCompatibleCatalogOperations; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; @@ -42,7 +42,7 @@ public class OceanBaseCatalog extends JdbcCatalog { @Override protected CatalogOperations newOps(Map<String, String> config) { - return new JdbcCatalogOperations( + return new MySQLProtocolCompatibleCatalogOperations( createExceptionConverter(), createJdbcTypeConverter(), createJdbcDatabaseOperations(), diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java index b70abc354d..e9716749fa 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java @@ -102,6 +102,7 @@ public class JdbcCatalogOperations implements CatalogOperations, SupportsSchemas private final TableOperation tableOperation; private DataSource dataSource; + private String jdbcUrl; private final JdbcColumnDefaultValueConverter columnDefaultValueConverter; @@ -170,6 +171,7 @@ public class JdbcCatalogOperations implements CatalogOperations, SupportsSchemas resultConf.putAll(gravitinoConfig); JdbcConfig jdbcConfig = new JdbcConfig(resultConf); + this.jdbcUrl = jdbcConfig.getJdbcUrl(); this.dataSource = DataSourceUtils.createDataSource(jdbcConfig); checkJDBCDriverVersion(); @@ -199,6 +201,28 @@ public class JdbcCatalogOperations implements CatalogOperations, SupportsSchemas metricsSystem.unregister(catalogMetricsSource); } DataSourceUtils.closeDataSource(dataSource); + try { + Driver driver = getDriver(); + if (driver != null) { + deregisterDriver(driver); + } + } catch (SQLException e) { + LOG.warn("Failed to deregister JDBC driver", e); + } + } + + /** + * Gets the JDBC driver for the provided JDBC URL. + * + * @return The JDBC driver instance if the JDBC URL is not blank; null if the JDBC URL is blank. + * @throws SQLException if failing to get driver from JDBC URL. + */ + protected Driver getDriver() throws SQLException { + if (StringUtils.isBlank(jdbcUrl)) { + return null; + } + + return DriverManager.getDriver(jdbcUrl); } /** diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/MySQLProtocolCompatibleCatalogOperations.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/MySQLProtocolCompatibleCatalogOperations.java index b4ee565dce..2a5bddaa99 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/MySQLProtocolCompatibleCatalogOperations.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/MySQLProtocolCompatibleCatalogOperations.java @@ -21,6 +21,7 @@ package org.apache.gravitino.catalog.jdbc; import java.sql.Driver; import java.sql.DriverManager; +import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; @@ -70,13 +71,13 @@ public class MySQLProtocolCompatibleCatalogOperations extends JdbcCatalogOperati .getMethod("uncheckedShutdown") .invoke(null); LOG.info("AbandonedConnectionCleanupThread has been shutdown..."); - - // Unload the MySQL driver, only Unload the driver if it is loaded by - // IsolatedClassLoader. - Driver mysqlDriver = DriverManager.getDriver("jdbc:mysql://dumpy_address"); - deregisterDriver(mysqlDriver); } catch (Exception e) { - LOG.warn("Failed to shutdown AbandonedConnectionCleanupThread or deregister MySQL driver", e); + LOG.warn("Failed to shutdown AbandonedConnectionCleanupThread", e); } } + + @Override + protected Driver getDriver() throws SQLException { + return DriverManager.getDriver("jdbc:mysql://dummy_address"); + } } diff --git a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java index 0b082d5916..d7f711e20f 100644 --- a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java +++ b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java @@ -20,6 +20,8 @@ package org.apache.gravitino.catalog.jdbc; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; +import java.sql.Driver; +import java.sql.DriverManager; import java.sql.SQLException; import java.util.HashMap; import javax.sql.DataSource; @@ -27,6 +29,8 @@ import org.apache.commons.dbcp2.BasicDataSource; import org.apache.gravitino.Catalog; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.catalog.jdbc.config.JdbcConfig; +import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; +import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.converter.SqliteColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.SqliteExceptionConverter; import org.apache.gravitino.catalog.jdbc.converter.SqliteTypeConverter; @@ -75,4 +79,78 @@ public class TestJdbcCatalogOperations { Assertions.assertFalse(((BasicDataSource) dataSource).getTestOnBorrow()); ((BasicDataSource) dataSource).close(); } + + @Test + public void testCloseDeregisterDriver() throws SQLException { + TestableJdbcCatalogOperations catalogOperations = + new TestableJdbcCatalogOperations( + new SqliteExceptionConverter(), + new SqliteTypeConverter(), + new SqliteDatabaseOperations("/illegal/path"), + new SqliteTableOperations(), + new SqliteColumnDefaultValueConverter()); + catalogOperations.setDriver(DriverManager.getDriver("jdbc:sqlite::memory:")); + + Assertions.assertDoesNotThrow(catalogOperations::close); + Assertions.assertTrue(catalogOperations.isDeregisterCalled()); + } + + @Test + public void testCloseIgnoreGetDriverException() { + TestableJdbcCatalogOperations catalogOperations = + new TestableJdbcCatalogOperations( + new SqliteExceptionConverter(), + new SqliteTypeConverter(), + new SqliteDatabaseOperations("/illegal/path"), + new SqliteTableOperations(), + new SqliteColumnDefaultValueConverter()); + catalogOperations.setThrowExceptionInGetDriver(true); + + Assertions.assertDoesNotThrow(catalogOperations::close); + } + + private static class TestableJdbcCatalogOperations extends JdbcCatalogOperations { + private Driver driver; + private boolean deregisterCalled; + private boolean throwExceptionInGetDriver; + + private TestableJdbcCatalogOperations( + JdbcExceptionConverter exceptionConverter, + JdbcTypeConverter jdbcTypeConverter, + SqliteDatabaseOperations databaseOperation, + SqliteTableOperations tableOperation, + SqliteColumnDefaultValueConverter columnDefaultValueConverter) { + super( + exceptionConverter, + jdbcTypeConverter, + databaseOperation, + tableOperation, + columnDefaultValueConverter); + } + + @Override + protected Driver getDriver() throws SQLException { + if (throwExceptionInGetDriver) { + throw new SQLException("failed to get driver"); + } + return driver; + } + + @Override + public void deregisterDriver(Driver driver) { + this.deregisterCalled = true; + } + + private void setDriver(Driver driver) { + this.driver = driver; + } + + private void setThrowExceptionInGetDriver(boolean throwExceptionInGetDriver) { + this.throwExceptionInGetDriver = throwExceptionInGetDriver; + } + + private boolean isDeregisterCalled() { + return deregisterCalled; + } + } } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java index 527a27072a..b04ac0dad0 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java @@ -20,6 +20,7 @@ package org.apache.gravitino.catalog.postgresql; import java.sql.Driver; import java.sql.DriverManager; +import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.JdbcCatalogOperations; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; @@ -44,15 +45,7 @@ public class PostgreSQLCatalogOperations extends JdbcCatalogOperations { } @Override - public void close() { - super.close(); - try { - // Unload the PostgreSQL driver, only Unload the driver if it is loaded by - // IsolatedClassLoader. - Driver pgDriver = DriverManager.getDriver("jdbc:postgresql://dummy_address:12345/"); - deregisterDriver(pgDriver); - } catch (Exception e) { - LOG.warn("Failed to deregister PostgreSQL driver", e); - } + protected Driver getDriver() throws SQLException { + return DriverManager.getDriver("jdbc:postgresql://dummy_address:12345/"); } }
