[refactor](catalog) fe-core: delete the unreachable metastore property cluster and the dead storage/AWS-credential surface - #66210
Merged
Conversation
Records the investigation into whether fe-core
datasource/property/{common,metastore} can be deleted or relocated to
fe-connector / fe-filesystem, and the resulting plan.
Verdicts differ per package and must not be treated as one job:
- metastore/ (4 files, 333 lines) plus the orphaned ConnectionProperties
is unreachable dead code. Its registry holds only TRINO_CONNECTOR,
whose factory never parses anything; both doors into it are
unreachable; nothing there is Gson-persisted; and its successor
(fe-connector-metastore-api MetaStoreProperties + MetaStoreProviders +
Connector.deriveStorageProperties) is already live. Nothing needs to
be migrated -- only deleted.
- common/ (2 files, 237 lines) serves internal storage (cold-storage
StoragePolicy, cloud StorageVault, TVF/backup/export), not external
datasources. No fe-connector or fe-filesystem module imports it, every
relocation target is dependency- or classloader-illegal, and reusing
the fe-filesystem-s3-base twin is a behavior change rather than a
refactor. Only its provably dead half is removed.
The space follows the hive-catalog-shade-removal layout and adds an
open-decisions file, since one decision (fail-loud vs fail-silent on the
null-supplier branch) gates the main deletion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deletes org.apache.doris.datasource.property.metastore (4 files) and the ConnectionProperties base it orphans. Nothing moves: the successor is already live connector-side (fe-connector-metastore-api MetaStoreProperties, fe-connector-metastore-spi MetaStoreProviders.bind, and Connector.deriveStorageProperties), so this is deletion, not migration. The cluster was unreachable. Its registry held only TRINO_CONNECTOR, whose factory returns a bare MetastoreProperties without ever calling initNormalizeAndCheckProps -- so it parsed nothing, and createInternal, getDerivedStorageProperties, getExecutionAuthenticator and the StorageAuthenticatorBridge all had zero callers. CatalogProperty was the sole importer and offered two doors: checkMetaStoreAndStorageProperties (no callers repo-wide) and resolveDerivedStorageDefaults, whose metastore branch runs only when pluginDerivedStorageDefaultsSupplier is null -- and PluginDrivenExternalCatalog installs that supplier unconditionally. The other two ExternalCatalog subclasses left in fe-core never touch the storage path. Nothing here was Gson-persisted, so no image or replay compatibility is at stake; also note MetastoreProperties.Type is unrelated to the persisted InitCatalogLog.Type/InitDatabaseLog.Type constants of the same name. resolveDerivedStorageDefaults now throws when the supplier is unwired instead of deriving nothing. That preserves today's behavior, where the metastore parse threw on this path. Deriving an empty map would silently drop the iceberg warehouse -> fs.defaultFS bridge and, because the setter deliberately does not reset caches, would cache the under-derived StorageBindings for good. CatalogPropertyPluginStorageDerivationTest gains a case pinning that, and its stale mutation comments are retargeted at mutations that still exist. Three dangling comment references are reworded. The one in StorageAdapter is javadoc prose, not a compile dependency. Verified: full reactor clean test-compile including test sources; fe-core checkstyle 0 violations; 95 fe-core tests including the Gson replay suites; 110 fe-connector-api tests with connector-metadata-methods.txt unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The task space assumed StorageAdapter.getAwsCredentialsProvider() was simply dead code and planned to delete it, gated on first checking apache/doris master. That check now ran, and the assumption was wrong: upstream master (2faf819) has two live callers, connectivity/AbstractS3CompatibleConnectivityTester and property/common/IcebergAwsClientCredentialsProperties. It reads as dead here only because this branch's migration already deleted both consumers along with the whole datasource/connectivity package. Since StorageAdapter itself exists on both sides and is three-way merged on every rebase, removing the method would turn every upstream edit to that region into a manual conflict, in exchange for 146 lines that never execute. The recommendation is flipped to "do not delete" and the task is left blocked on the owner rather than carried out. No code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e arm StorageAdapter.getAwsCredentialsProvider() and its two helpers build a live AWS SDK v2 credentials object for FE-side SDK clients. Their consumers -- the catalog connectivity testers and IcebergAwsClientCredentialsProperties -- were removed earlier in this migration along with the whole datasource/connectivity package, so nothing calls them here anymore. The same goes for AwsCredentialsProviderFactory.createV2, createDefaultV2 and the single-arg getV2ClassName, which existed only to serve that arm. Note this is live on apache/doris master, where both consumers still exist; it reads as dead only on this branch. Removing it is therefore a deliberate trade, accepted by the owner: future upstream edits to this region will surface as modify/delete conflicts on rebase, to be resolved by keeping the deletion. Deliberately kept, because they are not part of that arm: getAwsCredentialsProviderMode() and the s3CredentialsMode field, and AwsCredentialsProviderFactory.getV2ClassName(mode, boolean) with its two env probes. Those emit the provider class-name string that reaches BE via AWS_CREDENTIALS_PROVIDER_TYPE and the hadoop fs.s3a.aws.credentials.provider map, and AzureGuessRoutingParityTest pins the mode accessor. Orphaned imports were determined per symbol rather than from the plan, which also confirmed InstanceProfileCredentialsProvider and Config are still used. Three connector comments naming the deleted methods are reworded; the iceberg twin is now the only implementation of the provider-instance mapping. Verified: full reactor clean test-compile including test sources; fe-core checkstyle 0 violations; 52 storage-adapter and catalog-property tests pass (1 pre-existing @disabled in LocationPathTest). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured while running the full fe-core suite to verify a base-class change: it was still going at 3h20m, and just under half that time was spent actually running tests. Root causes, with evidence rather than guesswork: - Every test class forks a fresh JVM. fe-core/pom.xml sets reuseForks=false, deliberately, to avoid singleton conflicts -- so this is not a subset of tests and not something to simply flip. - forkCount comes from fe_ut_parallel, which defaults to 1 and is only overridable through the FE_UT_PARALLEL environment variable. It was unset, so 15 of the machine's 16 cores sat idle. The pom comment even suggests raising it. - Roughly 5.5s of JVM startup per class, against 1225 classes, a third of which hold only one or two test cases. - nereids accounts for 72% of test execution time and is unrelated to the change that required the run. The note records data and preliminary directions only. Raising FE_UT_PARALLEL is untested here and carries the real risk that concurrent forks contend over ports, BDB directories and temp paths; the compile/test split behind the 5.5s figure is estimated, not measured. Both are called out as open. Filed standalone because the owner will handle it on a separate branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note compared against a hand-written maven invocation. The official pipeline does not use that: TeamCity Doris_Doris_FeUt runs a container that invokes the repo's own run-fe-ut.sh --coverage, which expands to a -Pcoverage test jacoco:report run over FE_MODULES. Also records, without drawing conclusions, three differences that matter when reading CI results: the pipeline additionally produces coverage, it passes -Dmaven.test.failure.ignore=true and decides pass/fail in the surrounding shell, and that shell forces the exit code back to 0 when a run has more than 6000 tests with at most 10 failures and 10 errors -- so a green FeUt build is not the same as zero failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…alog base Deletes five callerless members from ExternalCatalog -- getHadoopProperties, getConfiguration, buildConf, buildHadoopConfiguration and ifNotSetFallbackToSimpleAuth -- along with the cachedConf/confLock fields they cached through, and three from CatalogProperty: getHadoopProperties, getBackendStorageProperties and getOrderedStorageAdapters, with their two lazily-populated fields. getConfiguration was already marked deprecated "until connector SPI extraction is complete", which it now is. Re-grepping before the cut corrected the plan twice. It had said not to touch buildHadoopConfiguration because its callers were never enumerated; enumerating them shows all sixteen hits are the connector-side IcebergCatalogFactory and PaimonCatalogFactory methods of the same name, in different classes, so the ExternalCatalog one is dead too. The plan had also missed ifNotSetFallbackToSimpleAuth, whose only two uses were inside the methods being deleted, and missed that cachedConf/confLock are also touched outside getConfiguration -- in resetToUninitialized and in the Gson post-process -- which a field-declaration-only deletion would have broken. The point is not the 135 lines. initStorageAdapters is now reachable only through getStorageAdaptersMap and getEffectiveRawStorageProperties, both called from PluginDrivenExternalCatalog, so "fe-core storage has a single entrance" holds by construction rather than by audit -- which is also what backs the fail-loud branch added when the metastore cluster was retired. Verified: repo-wide grep clean; full reactor clean test-compile including test sources; fe-core checkstyle 0 violations. The full fe-core suite was stopped by the owner at 3h29m after 1232 test classes rather than run to completion, so the test claim is "no failures among the 1232 classes executed", not "the suite passes". Its one failure, ForwardToMasterTest.testAddBeDropBe, reproduces identically on a clean HEAD with these changes stashed, so it is pre-existing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
added a commit
that referenced
this pull request
Jul 29, 2026
…y cluster and the dead storage/AWS-credential surface (#66210) ### What problem does this PR solve? Issue Number: #65185 Problem Summary: Part of the catalog-SPI migration (#65185). Review round 21, targeting `branch-catalog-spi`. This round is one self-contained work line: finish the architectural goal "**`fe-core` holds no property parsing**" for `org.apache.doris.datasource.property`. Everything here is deletion of code that is unreachable on this branch — nothing moves and nothing is migrated, because every successor is already live connector-side. `fe-core` loses ~860 lines and gains none. The investigation, the per-package verdicts, the rejected alternatives and the rolling handoff are included under `plan-doc/fecore-property-cleanup/`. The two packages under `datasource/property/` were investigated together but got **different** verdicts, and are deliberately not treated as one job: | package | verdict | reason | | --- | --- | --- | | `metastore/` (4 files, 333 lines) + the orphaned `ConnectionProperties` | deleted outright | unreachable at runtime; its successor has been live for some time — nothing needs to move | | `common/` (2 files, 237 lines) | **stays in `fe-core`**, only its provably dead half is cut | it serves *internal* storage (cold-storage `StoragePolicy`, cloud `StorageVault`), not external datasources; every relocation target is dependency- or classloader-illegal | **1. The metastore property cluster is deleted, not migrated** `datasource/property/metastore/` (4 files) plus the `ConnectionProperties` base it orphans. The successor is already running: `fe-connector-metastore-api` `MetaStoreProperties`, `fe-connector-metastore-spi` `MetaStoreProviders.bind` and `Connector.deriveStorageProperties`. The cluster was unreachable. Its registry held only `TRINO_CONNECTOR`, whose factory returns a bare `MetastoreProperties` without ever calling `initNormalizeAndCheckProps` — so it parsed nothing, and `createInternal`, `getDerivedStorageProperties`, `getExecutionAuthenticator` and the `StorageAuthenticatorBridge` all had zero callers. `CatalogProperty` was the sole importer and offered two doors: `checkMetaStoreAndStorageProperties` (no callers repo-wide) and `resolveDerivedStorageDefaults`, whose metastore branch runs only when `pluginDerivedStorageDefaultsSupplier` is null — and `PluginDrivenExternalCatalog` installs that supplier unconditionally. Nothing here was Gson-persisted, so no image or replay compatibility is at stake (note `MetastoreProperties.Type` is unrelated to the persisted `InitCatalogLog.Type` / `InitDatabaseLog.Type` constants of the same name). `resolveDerivedStorageDefaults` now **throws** when the supplier is unwired instead of deriving nothing. That preserves today's behavior, where the metastore parse threw on this path. Deriving an empty map would silently drop the iceberg `warehouse` → `fs.defaultFS` bridge and, because the setter deliberately does not reset caches, would cache the under-derived `StorageBindings` for good. `CatalogPropertyPluginStorageDerivationTest` gains a case pinning that, and its stale mutation comments are retargeted at mutations that still exist. **2. The callerless AWS provider-instance arm is dropped** `StorageAdapter.getAwsCredentialsProvider()` and its two helpers build a live AWS SDK v2 credentials object for FE-side SDK clients; `AwsCredentialsProviderFactory.createV2`, `createDefaultV2` and the single-arg `getV2ClassName` existed only to serve that arm. Both consumers — the catalog connectivity testers and `IcebergAwsClientCredentialsProperties` — were removed earlier in this migration along with the whole `datasource/connectivity` package, so nothing calls them here anymore. **This one is a deliberate trade, not merely dead code.** It is still live on `apache/doris` master, where both consumers exist; it reads as dead only on this branch. Removing it means future upstream edits to that region will surface as modify/delete conflicts on rebase, to be resolved by keeping the deletion. The investigation first flipped its own recommendation to *do not delete* for exactly that reason (see the `[doc](catalog) record that the dead AWS provider arm is live upstream` commit); the owner then accepted the trade, and only after that was the deletion carried out. Deliberately **kept**, because they are not part of that arm: `getAwsCredentialsProviderMode()` and the `s3CredentialsMode` field, and `AwsCredentialsProviderFactory.getV2ClassName(mode, boolean)` with its two env probes. Those emit the provider class-name string that reaches BE via `AWS_CREDENTIALS_PROVIDER_TYPE` and the hadoop `fs.s3a.aws.credentials.provider` map, and `AzureGuessRoutingParityTest` pins the mode accessor. Folding them into the look-alike `fe-filesystem-s3-base` twin was considered and **rejected**: the two implementations differ in two live behaviors (the hadoop string gains a `ProfileCredentialsProvider`; the accepted mode strings widen), and no test in the repo pins the emitted string — so the swap would have shipped a regression green. Three connector comments naming the deleted methods are reworded; the iceberg twin is now the only implementation of the provider-instance mapping. **3. The dead storage doors on the catalog base are removed** Five callerless members of `ExternalCatalog` — `getHadoopProperties`, `getConfiguration`, `buildConf`, `buildHadoopConfiguration`, `ifNotSetFallbackToSimpleAuth` — together with the `cachedConf` / `confLock` fields they cached through, and three of `CatalogProperty` — `getHadoopProperties`, `getBackendStorageProperties`, `getOrderedStorageAdapters` — with their two lazily populated fields. `getConfiguration` was already marked deprecated "until connector SPI extraction is complete", which it now is. The point is not the 135 lines. `initStorageAdapters` is now reachable only through `getStorageAdaptersMap` and `getEffectiveRawStorageProperties`, both called from `PluginDrivenExternalCatalog`, so "**`fe-core` storage has a single entrance**" holds by construction rather than by audit — which is also what backs the fail-loud branch in item 1. **4. Two test-infrastructure notes (no code)** `plan-doc/fe-core-ut-runtime-problem.md` records, with measurements rather than guesswork, why a full `fe-core` run takes 3h+: `reuseForks=false` (deliberate, to avoid singleton conflicts) means a fresh JVM per class at ~5.5s startup over 1225 classes, `forkCount` comes from `fe_ut_parallel` which defaults to 1 unless `FE_UT_PARALLEL` is set, and nereids accounts for 72% of test execution time. It also records what the FE UT pipeline actually runs — TeamCity `Doris_Doris_FeUt` invokes the repo's own `run-fe-ut.sh --coverage`, passes `-Dmaven.test.failure.ignore=true`, and forces the exit code back to 0 when a run has more than 6000 tests with at most 10 failures and 10 errors, so a green FeUt build is not the same as zero failures. Both notes are directions only, filed for a separate branch — raising `FE_UT_PARALLEL` is untested and risks fork contention over ports, BDB directories and temp paths. ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> Local verification, stated at the coverage it actually has: - repo-wide grep for every deleted symbol: clean. - full reactor **clean** `test-compile` **including test sources** (no `-Dmaven.test.skip`), after each of the four deletions: BUILD SUCCESS. Deletion changes cannot be trusted to incremental compilation, so `fe-core/target/{classes,test-classes}` was removed before each run. - `fe-core` `checkstyle:check`: 0 violations. - targeted suites: 95 `fe-core` tests including the Gson replay suites; 110 `fe-connector-api` tests with `connector-metadata-methods.txt` unchanged; 52 storage-adapter and catalog-property tests (1 pre-existing `@Disabled` in `LocationPathTest`). - **the full `fe-core` suite was not run to completion.** It was still going at 3h29m after 1232 test classes and was stopped by the owner rather than finished, so the claim is "no failures among the 1232 classes executed", **not** "the suite passes". Its one failure, `ForwardToMasterTest.testAddBeDropBe` (`ClassCastException: JSONObject → JSONArray`), reproduces identically on a clean HEAD with these changes stashed, so it is pre-existing. - **no e2e was run** (no cluster available here); this PR adds no regression case. All four deletions remove unreachable code, and the storage-binding path they sit next to (iceberg hadoop `warehouse` → `fs.defaultFS`) is covered by unit tests only. CI is relied on for the rest. - no BE file is touched by this PR, so BE was not rebuilt. - Behavior changed: - [x] No. Every deleted member is callerless on this branch. The one semantic change — `resolveDerivedStorageDefaults` throwing when `pluginDerivedStorageDefaultsSupplier` is unwired — is not reachable in production wiring, since `PluginDrivenExternalCatalog` installs that supplier unconditionally, and it replaces a path that already threw. The one accepted forward cost is stated in item 2: modify/delete rebase conflicts against upstream master in the `StorageAdapter` region. - Does this need documentation? - [x] No. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
morningman
added a commit
that referenced
this pull request
Jul 29, 2026
…y cluster and the dead storage/AWS-credential surface (#66210) ### What problem does this PR solve? Issue Number: #65185 Problem Summary: Part of the catalog-SPI migration (#65185). Review round 21, targeting `branch-catalog-spi`. This round is one self-contained work line: finish the architectural goal "**`fe-core` holds no property parsing**" for `org.apache.doris.datasource.property`. Everything here is deletion of code that is unreachable on this branch — nothing moves and nothing is migrated, because every successor is already live connector-side. `fe-core` loses ~860 lines and gains none. The investigation, the per-package verdicts, the rejected alternatives and the rolling handoff are included under `plan-doc/fecore-property-cleanup/`. The two packages under `datasource/property/` were investigated together but got **different** verdicts, and are deliberately not treated as one job: | package | verdict | reason | | --- | --- | --- | | `metastore/` (4 files, 333 lines) + the orphaned `ConnectionProperties` | deleted outright | unreachable at runtime; its successor has been live for some time — nothing needs to move | | `common/` (2 files, 237 lines) | **stays in `fe-core`**, only its provably dead half is cut | it serves *internal* storage (cold-storage `StoragePolicy`, cloud `StorageVault`), not external datasources; every relocation target is dependency- or classloader-illegal | **1. The metastore property cluster is deleted, not migrated** `datasource/property/metastore/` (4 files) plus the `ConnectionProperties` base it orphans. The successor is already running: `fe-connector-metastore-api` `MetaStoreProperties`, `fe-connector-metastore-spi` `MetaStoreProviders.bind` and `Connector.deriveStorageProperties`. The cluster was unreachable. Its registry held only `TRINO_CONNECTOR`, whose factory returns a bare `MetastoreProperties` without ever calling `initNormalizeAndCheckProps` — so it parsed nothing, and `createInternal`, `getDerivedStorageProperties`, `getExecutionAuthenticator` and the `StorageAuthenticatorBridge` all had zero callers. `CatalogProperty` was the sole importer and offered two doors: `checkMetaStoreAndStorageProperties` (no callers repo-wide) and `resolveDerivedStorageDefaults`, whose metastore branch runs only when `pluginDerivedStorageDefaultsSupplier` is null — and `PluginDrivenExternalCatalog` installs that supplier unconditionally. Nothing here was Gson-persisted, so no image or replay compatibility is at stake (note `MetastoreProperties.Type` is unrelated to the persisted `InitCatalogLog.Type` / `InitDatabaseLog.Type` constants of the same name). `resolveDerivedStorageDefaults` now **throws** when the supplier is unwired instead of deriving nothing. That preserves today's behavior, where the metastore parse threw on this path. Deriving an empty map would silently drop the iceberg `warehouse` → `fs.defaultFS` bridge and, because the setter deliberately does not reset caches, would cache the under-derived `StorageBindings` for good. `CatalogPropertyPluginStorageDerivationTest` gains a case pinning that, and its stale mutation comments are retargeted at mutations that still exist. **2. The callerless AWS provider-instance arm is dropped** `StorageAdapter.getAwsCredentialsProvider()` and its two helpers build a live AWS SDK v2 credentials object for FE-side SDK clients; `AwsCredentialsProviderFactory.createV2`, `createDefaultV2` and the single-arg `getV2ClassName` existed only to serve that arm. Both consumers — the catalog connectivity testers and `IcebergAwsClientCredentialsProperties` — were removed earlier in this migration along with the whole `datasource/connectivity` package, so nothing calls them here anymore. **This one is a deliberate trade, not merely dead code.** It is still live on `apache/doris` master, where both consumers exist; it reads as dead only on this branch. Removing it means future upstream edits to that region will surface as modify/delete conflicts on rebase, to be resolved by keeping the deletion. The investigation first flipped its own recommendation to *do not delete* for exactly that reason (see the `[doc](catalog) record that the dead AWS provider arm is live upstream` commit); the owner then accepted the trade, and only after that was the deletion carried out. Deliberately **kept**, because they are not part of that arm: `getAwsCredentialsProviderMode()` and the `s3CredentialsMode` field, and `AwsCredentialsProviderFactory.getV2ClassName(mode, boolean)` with its two env probes. Those emit the provider class-name string that reaches BE via `AWS_CREDENTIALS_PROVIDER_TYPE` and the hadoop `fs.s3a.aws.credentials.provider` map, and `AzureGuessRoutingParityTest` pins the mode accessor. Folding them into the look-alike `fe-filesystem-s3-base` twin was considered and **rejected**: the two implementations differ in two live behaviors (the hadoop string gains a `ProfileCredentialsProvider`; the accepted mode strings widen), and no test in the repo pins the emitted string — so the swap would have shipped a regression green. Three connector comments naming the deleted methods are reworded; the iceberg twin is now the only implementation of the provider-instance mapping. **3. The dead storage doors on the catalog base are removed** Five callerless members of `ExternalCatalog` — `getHadoopProperties`, `getConfiguration`, `buildConf`, `buildHadoopConfiguration`, `ifNotSetFallbackToSimpleAuth` — together with the `cachedConf` / `confLock` fields they cached through, and three of `CatalogProperty` — `getHadoopProperties`, `getBackendStorageProperties`, `getOrderedStorageAdapters` — with their two lazily populated fields. `getConfiguration` was already marked deprecated "until connector SPI extraction is complete", which it now is. The point is not the 135 lines. `initStorageAdapters` is now reachable only through `getStorageAdaptersMap` and `getEffectiveRawStorageProperties`, both called from `PluginDrivenExternalCatalog`, so "**`fe-core` storage has a single entrance**" holds by construction rather than by audit — which is also what backs the fail-loud branch in item 1. **4. Two test-infrastructure notes (no code)** `plan-doc/fe-core-ut-runtime-problem.md` records, with measurements rather than guesswork, why a full `fe-core` run takes 3h+: `reuseForks=false` (deliberate, to avoid singleton conflicts) means a fresh JVM per class at ~5.5s startup over 1225 classes, `forkCount` comes from `fe_ut_parallel` which defaults to 1 unless `FE_UT_PARALLEL` is set, and nereids accounts for 72% of test execution time. It also records what the FE UT pipeline actually runs — TeamCity `Doris_Doris_FeUt` invokes the repo's own `run-fe-ut.sh --coverage`, passes `-Dmaven.test.failure.ignore=true`, and forces the exit code back to 0 when a run has more than 6000 tests with at most 10 failures and 10 errors, so a green FeUt build is not the same as zero failures. Both notes are directions only, filed for a separate branch — raising `FE_UT_PARALLEL` is untested and risks fork contention over ports, BDB directories and temp paths. ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> Local verification, stated at the coverage it actually has: - repo-wide grep for every deleted symbol: clean. - full reactor **clean** `test-compile` **including test sources** (no `-Dmaven.test.skip`), after each of the four deletions: BUILD SUCCESS. Deletion changes cannot be trusted to incremental compilation, so `fe-core/target/{classes,test-classes}` was removed before each run. - `fe-core` `checkstyle:check`: 0 violations. - targeted suites: 95 `fe-core` tests including the Gson replay suites; 110 `fe-connector-api` tests with `connector-metadata-methods.txt` unchanged; 52 storage-adapter and catalog-property tests (1 pre-existing `@Disabled` in `LocationPathTest`). - **the full `fe-core` suite was not run to completion.** It was still going at 3h29m after 1232 test classes and was stopped by the owner rather than finished, so the claim is "no failures among the 1232 classes executed", **not** "the suite passes". Its one failure, `ForwardToMasterTest.testAddBeDropBe` (`ClassCastException: JSONObject → JSONArray`), reproduces identically on a clean HEAD with these changes stashed, so it is pre-existing. - **no e2e was run** (no cluster available here); this PR adds no regression case. All four deletions remove unreachable code, and the storage-binding path they sit next to (iceberg hadoop `warehouse` → `fs.defaultFS`) is covered by unit tests only. CI is relied on for the rest. - no BE file is touched by this PR, so BE was not rebuilt. - Behavior changed: - [x] No. Every deleted member is callerless on this branch. The one semantic change — `resolveDerivedStorageDefaults` throwing when `pluginDerivedStorageDefaultsSupplier` is unwired — is not reachable in production wiring, since `PluginDrivenExternalCatalog` installs that supplier unconditionally, and it replaces a path that already threw. The one accepted forward cost is stated in item 2: modify/delete rebase conflicts against upstream master in the `StorageAdapter` region. - Does this need documentation? - [x] No. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #65185
Problem Summary:
Part of the catalog-SPI migration (#65185). Review round 21, targeting
branch-catalog-spi.This round is one self-contained work line: finish the architectural goal "
fe-coreholds noproperty parsing" for
org.apache.doris.datasource.property. Everything here is deletion of codethat is unreachable on this branch — nothing moves and nothing is migrated, because every successor
is already live connector-side.
fe-coreloses ~860 lines and gains none. The investigation, theper-package verdicts, the rejected alternatives and the rolling handoff are included under
plan-doc/fecore-property-cleanup/.The two packages under
datasource/property/were investigated together but got differentverdicts, and are deliberately not treated as one job:
metastore/(4 files, 333 lines) + the orphanedConnectionPropertiescommon/(2 files, 237 lines)fe-core, only its provably dead half is cutStoragePolicy, cloudStorageVault), not external datasources; every relocation target is dependency- or classloader-illegal1. The metastore property cluster is deleted, not migrated
datasource/property/metastore/(4 files) plus theConnectionPropertiesbase it orphans. Thesuccessor is already running:
fe-connector-metastore-apiMetaStoreProperties,fe-connector-metastore-spiMetaStoreProviders.bindandConnector.deriveStorageProperties.The cluster was unreachable. Its registry held only
TRINO_CONNECTOR, whose factory returns a bareMetastorePropertieswithout ever callinginitNormalizeAndCheckProps— so it parsed nothing, andcreateInternal,getDerivedStorageProperties,getExecutionAuthenticatorand theStorageAuthenticatorBridgeall had zero callers.CatalogPropertywas the sole importer andoffered two doors:
checkMetaStoreAndStorageProperties(no callers repo-wide) andresolveDerivedStorageDefaults, whose metastore branch runs only whenpluginDerivedStorageDefaultsSupplieris null — andPluginDrivenExternalCataloginstalls thatsupplier unconditionally. Nothing here was Gson-persisted, so no image or replay compatibility is at
stake (note
MetastoreProperties.Typeis unrelated to the persistedInitCatalogLog.Type/InitDatabaseLog.Typeconstants of the same name).resolveDerivedStorageDefaultsnow throws when the supplier is unwired instead of derivingnothing. That preserves today's behavior, where the metastore parse threw on this path. Deriving an
empty map would silently drop the iceberg
warehouse→fs.defaultFSbridge and, because the setterdeliberately does not reset caches, would cache the under-derived
StorageBindingsfor good.CatalogPropertyPluginStorageDerivationTestgains a case pinning that, and its stale mutationcomments are retargeted at mutations that still exist.
2. The callerless AWS provider-instance arm is dropped
StorageAdapter.getAwsCredentialsProvider()and its two helpers build a live AWS SDK v2 credentialsobject for FE-side SDK clients;
AwsCredentialsProviderFactory.createV2,createDefaultV2and thesingle-arg
getV2ClassNameexisted only to serve that arm. Both consumers — the catalog connectivitytesters and
IcebergAwsClientCredentialsProperties— were removed earlier in this migration alongwith the whole
datasource/connectivitypackage, so nothing calls them here anymore.This one is a deliberate trade, not merely dead code. It is still live on
apache/dorismaster,where both consumers exist; it reads as dead only on this branch. Removing it means future upstream
edits to that region will surface as modify/delete conflicts on rebase, to be resolved by keeping the
deletion. The investigation first flipped its own recommendation to do not delete for exactly that
reason (see the
[doc](catalog) record that the dead AWS provider arm is live upstreamcommit); theowner then accepted the trade, and only after that was the deletion carried out.
Deliberately kept, because they are not part of that arm:
getAwsCredentialsProviderMode()andthe
s3CredentialsModefield, andAwsCredentialsProviderFactory.getV2ClassName(mode, boolean)withits two env probes. Those emit the provider class-name string that reaches BE via
AWS_CREDENTIALS_PROVIDER_TYPEand the hadoopfs.s3a.aws.credentials.providermap, andAzureGuessRoutingParityTestpins the mode accessor. Folding them into the look-alikefe-filesystem-s3-basetwin was considered and rejected: the two implementations differ in twolive behaviors (the hadoop string gains a
ProfileCredentialsProvider; the accepted mode stringswiden), and no test in the repo pins the emitted string — so the swap would have shipped a regression
green. Three connector comments naming the deleted methods are reworded; the iceberg twin is now the
only implementation of the provider-instance mapping.
3. The dead storage doors on the catalog base are removed
Five callerless members of
ExternalCatalog—getHadoopProperties,getConfiguration,buildConf,buildHadoopConfiguration,ifNotSetFallbackToSimpleAuth— together with thecachedConf/confLockfields they cached through, and three ofCatalogProperty—getHadoopProperties,getBackendStorageProperties,getOrderedStorageAdapters— with their two lazily populated fields.getConfigurationwas already marked deprecated "until connector SPI extraction is complete", whichit now is.
The point is not the 135 lines.
initStorageAdaptersis now reachable only throughgetStorageAdaptersMapandgetEffectiveRawStorageProperties, both called fromPluginDrivenExternalCatalog, so "fe-corestorage has a single entrance" holds by constructionrather than by audit — which is also what backs the fail-loud branch in item 1.
4. Two test-infrastructure notes (no code)
plan-doc/fe-core-ut-runtime-problem.mdrecords, with measurements rather than guesswork, why a fullfe-corerun takes 3h+:reuseForks=false(deliberate, to avoid singleton conflicts) means a freshJVM per class at ~5.5s startup over 1225 classes,
forkCountcomes fromfe_ut_parallelwhichdefaults to 1 unless
FE_UT_PARALLELis set, and nereids accounts for 72% of test execution time. Italso records what the FE UT pipeline actually runs — TeamCity
Doris_Doris_FeUtinvokes the repo'sown
run-fe-ut.sh --coverage, passes-Dmaven.test.failure.ignore=true, and forces the exit codeback to 0 when a run has more than 6000 tests with at most 10 failures and 10 errors, so a green FeUt
build is not the same as zero failures. Both notes are directions only, filed for a separate branch —
raising
FE_UT_PARALLELis untested and risks fork contention over ports, BDB directories and temppaths.
Release note
None
Check List (For Author)
Local verification, stated at the coverage it actually has:
repo-wide grep for every deleted symbol: clean.
full reactor clean
test-compileincluding test sources (no-Dmaven.test.skip), after eachof the four deletions: BUILD SUCCESS. Deletion changes cannot be trusted to incremental compilation,
so
fe-core/target/{classes,test-classes}was removed before each run.fe-corecheckstyle:check: 0 violations.targeted suites: 95
fe-coretests including the Gson replay suites; 110fe-connector-apitestswith
connector-metadata-methods.txtunchanged; 52 storage-adapter and catalog-property tests(1 pre-existing
@DisabledinLocationPathTest).the full
fe-coresuite was not run to completion. It was still going at 3h29m after 1232 testclasses and was stopped by the owner rather than finished, so the claim is "no failures among the
1232 classes executed", not "the suite passes". Its one failure,
ForwardToMasterTest.testAddBeDropBe(ClassCastException: JSONObject → JSONArray), reproducesidentically on a clean HEAD with these changes stashed, so it is pre-existing.
no e2e was run (no cluster available here); this PR adds no regression case. All four deletions
remove unreachable code, and the storage-binding path they sit next to (iceberg hadoop
warehouse→fs.defaultFS) is covered by unit tests only. CI is relied on for the rest.no BE file is touched by this PR, so BE was not rebuilt.
Behavior changed:
Every deleted member is callerless on this branch. The one semantic change —
resolveDerivedStorageDefaultsthrowing whenpluginDerivedStorageDefaultsSupplieris unwired — isnot reachable in production wiring, since
PluginDrivenExternalCataloginstalls that supplierunconditionally, and it replaces a path that already threw. The one accepted forward cost is stated in
item 2: modify/delete rebase conflicts against upstream master in the
StorageAdapterregion.