From 4ad16db7957c55c5b05ce87ec450f6d057d4e65b Mon Sep 17 00:00:00 2001 From: Calvin Kirs Date: Fri, 28 Mar 2025 12:09:00 +0800 Subject: [PATCH 1/3] [Fix](Catalog) Close system resources when dropping catalog ### Description: This PR ensures that system resources, such as thread pools, are properly closed when a catalog is dropped. Previously, these resources were not explicitly released, which could lead to potential resource leaks. ### Changes: Implemented close() method in the catalog to release system resources. Ensured that thread pools and other managed resources are properly shut down when dropping a catalog. Added necessary cleanup logic in the dropCatalog method. --- .../org/apache/doris/datasource/ExternalCatalog.java | 6 ++++++ .../doris/datasource/hive/HMSExternalCatalog.java | 11 +++++++++++ .../datasource/iceberg/IcebergExternalCatalog.java | 8 ++++++++ .../doris/datasource/iceberg/IcebergMetadataOps.java | 3 +++ 4 files changed, 28 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java index 071accf0e7738a..64ce7bb036a1c3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java @@ -727,6 +727,12 @@ public void onClose() { if (threadPoolWithPreAuth != null) { ThreadPoolManager.shutdownExecutorService(threadPoolWithPreAuth); } + if (null != preExecutionAuthenticator) { + preExecutionAuthenticator = null; + } + if (null != transactionManager) { + transactionManager = null; + } CatalogIf.super.onClose(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java index 8032dfbb1e2419..20b3198384d454 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java @@ -197,6 +197,17 @@ public void onRefresh(boolean invalidCache) { } } + @Override + public void onClose() { + super.onClose(); + if (null != fileSystemExecutor) { + ThreadPoolManager.shutdownExecutorService(fileSystemExecutor); + } + if (null != icebergMetadataOps) { + icebergMetadataOps.close(); + } + } + @Override public List listTableNames(SessionContext ctx, String dbName) { makeSureInitialized(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java index 82ae49152bac7d..9bb7ca8ae08659 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java @@ -98,6 +98,14 @@ public List listTableNames(SessionContext ctx, String dbName) { return metadataOps.listTableNames(dbName); } + @Override + public void onClose() { + super.onClose(); + if (null != catalog) { + catalog = null; + } + } + protected void initS3Param(Configuration conf) { Map properties = catalogProperty.getHadoopProperties(); conf.set(Constants.AWS_CREDENTIALS_PROVIDER, PropertyConverter.getAWSCredentialsProviders(properties)); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java index 11451b445a2ed9..b17ed653e833e6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java @@ -86,6 +86,9 @@ public ExternalCatalog getExternalCatalog() { @Override public void close() { + if (catalog != null) { + catalog = null; + } } @Override From f43c3ff3d5f9902febd0a93a291e83234532724d Mon Sep 17 00:00:00 2001 From: Calvin Kirs Date: Fri, 28 Mar 2025 19:22:29 +0800 Subject: [PATCH 2/3] rename on refresh method --- .../apache/doris/datasource/CatalogIf.java | 2 +- .../apache/doris/datasource/CatalogMgr.java | 2 +- .../doris/datasource/ExternalCatalog.java | 20 +++++++++++++++++-- .../datasource/hive/HMSExternalCatalog.java | 4 ++-- .../datasource/jdbc/JdbcExternalCatalog.java | 8 ++------ 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogIf.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogIf.java index 9b8334c39a8292..bedc52615b9e50 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogIf.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogIf.java @@ -94,7 +94,7 @@ default String getResource() { default void notifyPropertiesUpdated(Map updatedProps) { if (this instanceof ExternalCatalog) { - ((ExternalCatalog) this).onRefresh(false); + ((ExternalCatalog) this).resetToUninitialized(false); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java index ca8d3147bc9212..c40f0c6c8690f0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java @@ -125,7 +125,7 @@ private void addCatalog(CatalogIf catalog) { idToCatalog.put(catalog.getId(), catalog); String catalogName = catalog.getName(); if (!catalogName.equals(InternalCatalog.INTERNAL_CATALOG_NAME)) { - ((ExternalCatalog) catalog).onRefresh(false); + ((ExternalCatalog) catalog).resetToUninitialized(false); } if (!Strings.isNullOrEmpty(catalog.getResource())) { Resource resource = Env.getCurrentEnv().getResourceMgr().getResource(catalog.getResource()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java index 64ce7bb036a1c3..52ac5a28bda2eb 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java @@ -501,7 +501,23 @@ private List> getFilteredDatabaseNames() { return remoteToLocalPairs; } - public void onRefresh(boolean invalidCache) { + /** + * Resets the Catalog state to uninitialized, releases resources held by {@code initLocalObjectsImpl()} + *

+ * This method is typically invoked during operations such as {@code CREATE CATALOG} + * and {@code MODIFY CATALOG}. It marks the object as uninitialized, clears cached + * configurations, and ensures that resources allocated during {@link #initLocalObjectsImpl()} + * are properly released via {@link #onClose()} + *

+ *

+ * The {@code onClose()} method is responsible for cleaning up resources that were initialized + * in {@code initLocalObjectsImpl()}, preventing potential resource leaks. + *

+ * + * @param invalidCache if {@code true}, the catalog cache will be invalidated + * and reloaded during the refresh process. + */ + public void resetToUninitialized(boolean invalidCache) { this.objectCreated = false; this.initialized = false; synchronized (this.propLock) { @@ -511,6 +527,7 @@ public void onRefresh(boolean invalidCache) { synchronized (this.confLock) { this.cachedConf = null; } + onClose(); refreshOnlyCatalogCache(invalidCache); } @@ -733,7 +750,6 @@ public void onClose() { if (null != transactionManager) { transactionManager = null; } - CatalogIf.super.onClose(); } private void removeAccessController() { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java index 20b3198384d454..e40759456907de 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java @@ -190,8 +190,8 @@ protected void initLocalObjectsImpl() { } @Override - public void onRefresh(boolean invalidCache) { - super.onRefresh(invalidCache); + public void resetToUninitialized(boolean invalidCache) { + super.resetToUninitialized(invalidCache); if (metadataOps != null) { metadataOps.close(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalCatalog.java index 03554dafbcb940..b63685d9fb376c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalCatalog.java @@ -125,12 +125,8 @@ public void setDefaultPropsIfMissing(boolean isReplay) { } @Override - public void onRefresh(boolean invalidCache) { - super.onRefresh(invalidCache); - if (jdbcClient != null) { - jdbcClient.closeClient(); - jdbcClient = null; - } + public void resetToUninitialized(boolean invalidCache) { + super.resetToUninitialized(invalidCache); this.identifierMapping = new JdbcIdentifierMapping( (Env.isTableNamesCaseInsensitive() || Env.isStoredTableNamesLowerCase()), Boolean.parseBoolean(getLowerCaseMetaNames()), From f014751df33df50bd8c19ab678ed793135fd0aa5 Mon Sep 17 00:00:00 2001 From: Calvin Kirs Date: Mon, 31 Mar 2025 12:00:40 +0800 Subject: [PATCH 3/3] Fix NPE --- .../doris/mysql/privilege/AccessControllerManager.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java index 9047d402dc2523..121d5a1f394993 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java @@ -34,6 +34,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Maps; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -167,7 +168,12 @@ private String getPluginIdentifierForAccessController(String acClassName) { } public void removeAccessController(String ctl) { - ctlToCtlAccessController.remove(ctl); + if (StringUtils.isBlank(ctl)) { + return; + } + if (ctlToCtlAccessController.containsKey(ctl)) { + ctlToCtlAccessController.remove(ctl); + } LOG.info("remove access controller for catalog {}", ctl); }