Gabriel39 commented on PR #68585: URL: https://github.com/apache/doris/pull/68585#issuecomment-5888279496
Reviewed head `b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19`. Moving HA validation to DDL is useful, but I found two issues in the new validation path: ### 1. [P2] Relative Hadoop XML resources lose their configured base directory [`validateStorageProperties()`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java#L269-L277) passes the raw catalog map directly to `FileSystemFactory.bindAllStorageProperties()`. The HDFS properties implementation actually loads XML through [`HdfsCompatibleProperties.loadConfigFromFile()`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-filesystem/fe-filesystem-hdfs-base/src/main/java/org/apache/doris/filesystem/hdfs/properties/HdfsCompatibleProperties.java#L333-L340), which reads the `_HADOOP_CONFIG_DIR_` map entry. Unlike [`StorageAdapter.ofAll()`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageAdapter.java#L152-L157), the new path does not inject that entry. `FileSystemFactory` sets the `doris.hadoop.config.dir` system property, but the loader reached by this binding path does not consume it. Without the map entry, it resolves relative resource names against the process working directory. For a catalog with `hadoop.config.resources=core-site.xml,hdfs-site.xml`, with both files correctly installed under `Config.hadoop_config_dir`, CREATE/ALTER can therefore fail with `Config resource file does not exist`. Even an ALTER of an unrelated property revalidates the full candidate and hits this failure. Please reuse the existing directory-injection behavior or unify the binding entry points so validation and storage access resolve the same files. Add CREATE and ALTER tests using relative resource names under a temporary configured Hadoop directory. ### 2. [P2] CREATE validation failures leave the already-created connector unclosed The new check runs from [`CatalogMgr.createCatalogInternal()`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java#L553-L570), after `CatalogFactory.finishCatalogCreation()` has completed its failure-cleanup scope. At this point the Hive connector already exists; with connection testing enabled, it may also have an initialized HMS client pool. If HA validation throws, [`createCatalogImpl()`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java#L249-L268) only unlocks and propagates the exception. It does not call `catalog.onCreateFailure()` for this path. This leaves more than a temporary unreachable object: constructing `HiveConnector` registers a managed metadata cache in [`MetaCacheGovernance.CATALOG_CACHES`](https://github.com/apache/doris/blob/b011e9284b2b8f7bb1feb4af44d5c1ac7119bb19/fe/fe-connector/fe-connector-cache/src/main/java/org/apache/doris/connector/cache/MetaCacheGovernance.java#L58-L70), a process-wide strong-reference registry. Connector close is required to unregister it. Repeated CREATE statements rejected by the new HA check can accumulate these registrations, and any initialized HMS pool also misses explicit cleanup. The cleanup gap is in the existing lifecycle, but this PR introduces a new failure path through it. Please either perform storage validation inside the protected creation phase or ensure registration failures close the unregistered catalog outside the catalog lock. Add a lifecycle test that repeatedly rejects CREATE and verifies connector close and no growth in managed-cache registrations. This review was based on code inspection and call-chain analysis; I did not run builds or tests. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
