[#10253] Optimize driver degisteration logic for JDBC catalog to avoid possible OOM problem#10255
[#10253] Optimize driver degisteration logic for JDBC catalog to avoid possible OOM problem#10255yuqi1129 wants to merge 4 commits intoapache:mainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes JDBC driver deregistration in JdbcCatalogOperations.close() to reduce per-catalog duplication and help prevent driver/classloader leaks in long-running processes.
Changes:
- Add
jdbcUrltracking and agetDriver()hook inJdbcCatalogOperations, and deregister the driver duringclose(). - Update PostgreSQL/Hologres/ClickHouse/MySQL-protocol operations to rely on the base
close()and provide driver resolution viagetDriver(). - Add unit tests covering driver deregistration behavior and the “ignore getDriver failure during close” path; update OceanBase to use
MySQLProtocolCompatibleCatalogOperations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java | Centralizes driver lookup/deregistration in base close() using stored jdbcUrl. |
| catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/MySQLProtocolCompatibleCatalogOperations.java | Removes per-class driver deregistration and supplies a MySQL driver via getDriver(). |
| catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java | Removes per-class close() override and supplies driver via getDriver(). |
| catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/HologresCatalogOperations.java | Removes per-class close() override and supplies driver via getDriver() (PostgreSQL driver). |
| catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseCatalogOperations.java | Removes per-class close() override and supplies driver via getDriver(). |
| catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java | Adds UT coverage for base close() driver deregistration and exception tolerance. |
| catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java | Switches OceanBase to MySQL-protocol operations implementation. |
Comments suppressed due to low confidence (1)
catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/OceanBaseCatalog.java:50
OceanBaseCatalognow usesMySQLProtocolCompatibleCatalogOperations, which enforces a MySQL driver major version >= 8 (viacheckJDBCDriverVersion()) and runs MySQL-specific cleanup onclose(). Please confirm OceanBase is intended to require MySQL Connector/J 8+; otherwise this can break deployments using older MySQL-compatible drivers (notejdbc-oceanbase.confstill suggestscom.mysql.jdbc.Driver). If older drivers must remain supported, consider keepingJdbcCatalogOperationsor overridingcheckJDBCDriverVersion()for OceanBase.
protected CatalogOperations newOps(Map<String, String> config) {
return new MySQLProtocolCompatibleCatalogOperations(
createExceptionConverter(),
createJdbcTypeConverter(),
createJdbcDatabaseOperations(),
createJdbcTableOperations(),
createJdbcColumnDefaultValueConverter());
...talog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java
Outdated
Show resolved
Hide resolved
Code Coverage Report
Files
|
jerryshao
left a comment
There was a problem hiding this comment.
Code Review
Excellent refactor. Centralizing driver deregistration in JdbcCatalogOperations.close() via the getDriver() hook eliminates the duplicated override pattern across all subclasses.
Positive
- Fixes
System.err.printlninClickHouseCatalogOperations(now properly logged viaLOG.warnin the base class). - Fixes the typo
"dumpy_address"→"dummy_address"inMySQLProtocolCompatibleCatalogOperations. - Correctly moves
OceanBaseCatalogto useMySQLProtocolCompatibleCatalogOperations(MySQL-protocol-compatible). - The
TestableJdbcCatalogOperationstest pattern is clean and covers both the success and exception-swallowing paths.
Minor suggestion
MySQLProtocolCompatibleCatalogOperations.close() still contains the AbandonedConnectionCleanupThread.uncheckedShutdown() logic in a try/catch block — this is now separate from the driver deregistration (which moved to the base close()). This is fine architecturally, but a brief comment that the base close() handles driver deregistration would make the layering clearer for future readers.
LGTM. The minor suggestion above is optional.
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
JdbcCatalogOperationsclass, 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