[refactor](catalog) Unify external metadata cache framework - #66633
Merged
924060929 merged 10 commits intoAug 31, 2026
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
924060929
marked this pull request as ready for review
August 11, 2026 01:04
Contributor
TPC-H: Total hot run time: 28473 ms |
Contributor
TPC-DS: Total hot run time: 159564 ms |
Contributor
ClickBench: Total hot run time: 23.61 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for three production-reachable concurrency regressions in the unified metadata-cache publication paths:
- a cold FE database/table object becomes visible before its
IdNameIndexaction while the adapter reacquires its stripe; - an incremental database/table-name remap can overwrite a newer concurrent refresh snapshot; and
- a delayed refresh can overwrite a newer value loaded after eviction.
Checkpoint conclusions:
- Goal and proof: the catalog-local owner and hierarchical invalidation design largely accomplish the consolidation goal, and the new unit suites cover most load, invalidation, hierarchy, close, and pruning contracts. They do not cover the three accepted interleavings.
- Scope and design: this is a broad but cohesive cache-framework refactor. Existing mechanisms were reused appropriately, and no unrelated code change was found.
- Concurrency: remote loads stay outside gates/monitors and the gate/lock ordering showed no deadlock. The three inline findings are correctness gaps in cross-layer publication and refresh replacement.
- Lifecycle: owner close, detached-state cleanup, removal callbacks, bulk handles, and node/tombstone pruning were traced. The queued-refresh lease concern was dismissed because it had no distinct current production impact beyond the already-retained Runnable graph.
- Configuration and compatibility: no new configuration key, persistence format, FE-BE variable, or protocol was added. CacheSpec parsing, Maven/runtime closure, plugin classloading, and mixed old/new FE-plugin fallback showed no concrete compatibility defect.
- Parallel paths and conditions: database/table paths, all migrated connectors, Hive collection/value scopes, replay/DDL/event invalidations, Iceberg authorization-sensitive gates, and deliberate catalog-scoped exceptions were checked. No additional distinct issue remained.
- Tests and results: changed tests are deterministic and substantial, but latch coverage is missing for the three reported races. Per the review-runner instructions, no build or test command was run.
- Observability and performance: existing metrics/logging remain adequate for ordinary cache operations; remote I/O remains outside locks, and no separate material performance or observability defect was substantiated.
- Persistence, transactions, data writes, and storage/FE-BE compatibility: not applicable to this metadata-cache-only refactor.
There was no additional user-provided focus. The full 70-file authoritative diff was reviewed. After three rounds, all normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS; the review is converged with no unresolved or duplicate candidate.
| token = beginAction(stripe, key); | ||
| } | ||
| try { | ||
| V value = cached == null ? get(key) : cached; |
Contributor
There was a problem hiding this comment.
[P1] Keep a cold object hidden until its ID mapping is published
On a miss, get(key) installs the object in the shared MetaCache before this method reaches the pre-action hook and reacquires the FE stripe. If another same-stripe mutation holds that monitor, cache-only/name paths can already observe the object while ExternalCatalog.getDbNullable(id) or ExternalDatabase.getTableNullable(id) still reads an empty IdNameIndex and returns null. The base path also put immediately before the index action, so unlocked readers had a very small interval, but it acquired the stripe (and passed the test hook) before publishing either side; this refactor turns that into an interval that can block behind stripe contention. Please keep the value unpublished until the validated auxiliary action succeeds, and cover database/table by-ID lookups with a latch-based test.
| Runnable validation = Objects.requireNonNull(validationAction, "validationAction can not be null"); | ||
| StripeState<K> stripe = stripeState(key); | ||
| synchronized (stripe) { | ||
| V updated = effectiveEnabled ? remapper.apply(key, data.getIfPresent(key)) : null; |
Contributor
There was a problem hiding this comment.
[P1] Make name-snapshot remapping atomic with refresh
updated is derived from the current cached snapshot here, but the later invalidate/put pair is protected only by the FE stripe. Shared-runtime refresh does not take that stripe, so it can publish a newer snapshot N1 after this read; the remapper then invalidates N1 and installs U derived from the older N0, dropping any unrelated database/table names that arrived in the refresh until another full reload. Both database- and table-name caches enable auto-refresh, and their incremental add/drop paths use these remappers. Please add an exact-key atomic remap/compare-and-retry primitive so a concurrent refresh forces the function to re-evaluate the new value, with latch tests for both name caches.
| V refreshed = loadAndRecord(key, loader); | ||
| if (refreshed != null) { | ||
| synchronized (lease.keyNode) { | ||
| publishCommitted(lease, key, refreshed); |
Contributor
There was a problem hiding this comment.
[P1] Do not let an old refresh replace a newer miss result
This commit validates only the refresh lease's key/scope states, not that the wrapper which triggered the refresh is still current. A concrete production schedule is: refresh R starts from V1 and captures the old remote result; capacity/expiry evicts V1 without changing KeyState or loadPublicationState; a normal miss then loads and publishes V2 using those same identities; finally R reaches this line and overwrites V2 with stale V1. Bounded multi-key FE schema caches enable auto-refresh, so eviction makes this reachable. Please require the exact current wrapper/registration to remain current at refresh commit (or use expected-value conditional replacement), and add a latch test for eviction plus a newer miss.
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Aug 11, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66407 Problem Summary: The unified metadata cache still exposed duplicate invalidation ownership through CachingHmsClient, retained an engine registry and route abstraction for two built-in FE cache types, and used FE-only names that were easy to confuse with the connector cache API. This change makes the connector CatalogMetaCache the sole invalidation owner, publishes partition collection and partition invalidation atomically, removes redundant Hive and Hudi client type branches, and renames FE-only cache adapters and catalog runtimes to describe their actual roles. Remote Doris schema and backend caching remain catalog-scoped, while statistics caches keep their independent asynchronous semantics. ### Release note None ### Check List (For Author) - Test: Unit Test - Connector cache, HMS, Hive, and Hudi Maven unit tests - Targeted FE unit tests for cache runtime, naming cache, Remote Doris, external catalog/database, and refresh replay - Behavior changed: No. Internal cache ownership and invalidation publication are refactored without changing external SQL behavior. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Found one new blocking correctness issue, attached inline.
Review status: converged after two complete rounds. The second runtime, connector/FE, and risk-focused passes all returned NO_NEW_VALUABLE_FINDINGS after revalidating the accepted issue and the dismissed candidates.
Checkpoint conclusions:
- Correctness and concurrency: batched cold-leaf invalidation can let a pre-refresh HMS partition result publish afterward; this is the inline P1. Miss deduplication, exact-key and ancestor fencing, expected-wrapper refresh replacement, FE auxiliary-index publication, and pruning/removal paths otherwise held up.
- Lifecycle and cleanup: connector-owned
CatalogMetaCacheinstances, lazy HMS construction, close races, refresh teardown, removal callbacks, scope/key pruning, and sibling connector invalidation were traced without another production-reachable defect. - Configuration and compatibility: legacy Hive TTL mappings, independent per-entry enable/TTL/capacity controls, disabled-cache behavior, FE/plugin Caffeine linkage, aliases, and default-versus-Remote-Doris routing were checked. No storage/protocol, rolling FE/BE, persisted-state, or FE-BE variable compatibility change applies.
- Parallel consumers: ADBC, Hive/HMS/Hudi, Iceberg, MaxCompute, Paimon, FE schema/backends caches, local-versus-remote names, ancestor collection scopes, replay/drop/rename/refresh routes, and immutable Iceberg caches were reviewed. No second scope or routing mismatch survived verification.
- Tests and observability: the new hierarchy, concurrency, leak, disabled-cache, connector, and FE latch tests were inspected; the accepted issue lacks the cold batch-invalidation latch described inline. Cache metrics and failure logging remain available. No builds or tests were run because the authoritative review bundle explicitly prohibits them.
The three pre-existing inline P1 threads were rechecked against the current head and appear addressed: cold FE objects publish their ID mapping inside the guarded commit, name remaps retry exact-wrapper CAS, and refresh replaces only its expected wrapper.
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Aug 11, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66407 Problem Summary: The unified metadata cache still exposed duplicate invalidation ownership through CachingHmsClient, retained an engine registry and route abstraction for two built-in FE cache types, and used FE-only names that were easy to confuse with the connector cache API. This change makes the connector CatalogMetaCache the sole invalidation owner, publishes partition collection and partition invalidation atomically, removes redundant Hive and Hudi client type branches, and renames FE-only cache adapters and catalog runtimes to describe their actual roles. Remote Doris schema and backend caching remain catalog-scoped, while statistics caches keep their independent asynchronous semantics. ### Release note None ### Check List (For Author) - Test: Unit Test - Connector cache, HMS, Hive, and Hudi Maven unit tests - Targeted FE unit tests for cache runtime, naming cache, Remote Doris, external catalog/database, and refresh replay - Behavior changed: No. Internal cache ownership and invalidation publication are refactored without changing external SQL behavior. - Does this need documentation: No
924060929
force-pushed
the
prototype-scoped-metacache-ut
branch
from
August 11, 2026 03:46
2b2a1b6 to
04e684b
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: one distinct concurrency/lifecycle correctness issue remains.
Finding
[P2]An identity-preservingScopedMetaCache.compareAndSetstill installs a replacement wrapper. During aFeMetaCacheEntry.computeAfterValidationretry, this synchronously reports the exact still-currentExternalDatabaseas removed and resets its initialized table caches, ID index, and downstream database scope. The inline comment is attached atScopedMetaCache.java:293.
Critical checkpoint conclusions
- Goal and proof: the PR centralizes FE and connector metadata caches behind a catalog-scoped runtime with hierarchical invalidation, shared configuration, lifecycle, and statistics. The implementation and its added unit tests cover most load, refresh, invalidation, bulk-publication, close, and routing schedules, but no test combines an outer names-CAS retry, nested identity-preserving object publication, and the synchronous database removal listener; the accepted finding shows the goal is not fully met.
- Focus and scope:
review_focus.txtcontains no additional user-provided focus. All 85 changed files and their production callers/tests were reviewed. The refactor is broad but cohesive; compatibility adapters and connector-local wrappers keep the changes focused on metadata-cache unification. - Concurrency and locking: event/refresh/query threads, FE stripes, key monitors, publication gates, generations, and exact wrapper registrations were traced. Late load/refresh/bulk publication and cleanup are otherwise fenced. The accepted same-instance replacement callback is the remaining race. The proposed broader callback deadlock was dismissed because the database entry, table entries, and FE schema runtime use independent
CatalogMetaCacheowners/registries with no reverse lock edge. - Lifecycle and ownership: connector owners close before their remote resources, scoped registrations are pruned, and catalog remove/recreate paths close the appropriate runtime. No static-initialization issue or additional leak/close race was found.
- Configuration: legacy TTL overlays, non-positive TTL folding, independent enable/capacity controls, disabled-cache semantics, and ephemeral schema-cache property overlay were preserved. No new global dynamic configuration contract requires runtime propagation.
- Compatibility: cache API/module relocation, preserved signatures, plugin classloading fallbacks, default versus remote-Doris routing, aliases, stats identity, replay, and catalog remove/recreate behavior were checked. No FE-BE protocol or storage-format change is involved.
- Parallel paths and conditions: ADBC, Hive/HMS/Hudi, Iceberg, MaxCompute, Paimon, FE schema caches, DDL/event/replay paths, exact and collection invalidation, sibling forwarding, credentials, and time-travel variants were traced. No distinct missed path or unjustified condition remained.
- Tests and results: the PR adds extensive latch/barrier-based FE and cache unit tests, including the previously discussed cold ID publication, names remap, expected-wrapper refresh, and cold bulk-descendant races. The exact accepted removal-listener retry is missing. No generated result file is changed. Tests/builds were not run by this reviewer under the review-only bundle instructions.
- Error handling and data correctness: loader exception, null-as-miss, disabled-cache, conflict validation, and canonical partition-key behavior were checked. Aside from the accepted lifecycle reset, no silent error, stale publication, ID/name inconsistency, or incorrect invalidation survived review.
- Observability: cache metrics, per-entry statistics, sorting/deduplication, and existing lifecycle logging are sufficient for the new paths; no additional critical metric or identifier gap was found.
- Persistence, transactions, and writes: no transaction or user-data write path changes. Persisted catalog properties are not mutated by the derived overlay; EditLog/replay navigation and cold fallbacks were checked without a new persistence issue.
- FE-BE variables: no new variable or protocol field is passed between FE and BE.
- Memory safety and BE nullable handling: the change is FE Java metadata-cache code; BE allocator, C++ ownership, and nullable-column checkpoints are not applicable. Java cache ownership, defensive key copies, cleanup, and close were reviewed without another issue.
- Performance: gate scope, synchronous loaders/callbacks, single-flight admission, bounded caches, node pruning, metrics, and connector-specific cache granularity were checked. No distinct production performance regression or obvious redundant operation was substantiated.
- Other issues and duplicates: existing discussions
3754745907,3754745909,3754745910, and3755059016cover the fixed cold object/ID publication, names remap, refresh replacement, and cold bulk-descendant issues and were not duplicated.
Review status: converged after two full rounds. One inline comment is requested; every other candidate was duplicate-suppressed or dismissed with concrete code evidence.
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Aug 11, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66407 Problem Summary: The unified metadata cache still exposed duplicate invalidation ownership through CachingHmsClient, retained an engine registry and route abstraction for two built-in FE cache types, and used FE-only names that were easy to confuse with the connector cache API. This change makes the connector CatalogMetaCache the sole invalidation owner, publishes partition collection and partition invalidation atomically, removes redundant Hive and Hudi client type branches, and renames FE-only cache adapters and catalog runtimes to describe their actual roles. Remote Doris schema and backend caching remain catalog-scoped, while statistics caches keep their independent asynchronous semantics. ### Release note None ### Check List (For Author) - Test: Unit Test - Connector cache, HMS, Hive, and Hudi Maven unit tests - Targeted FE unit tests for cache runtime, naming cache, Remote Doris, external catalog/database, and refresh replay - Behavior changed: No. Internal cache ownership and invalidation publication are refactored without changing external SQL behavior. - Does this need documentation: No
924060929
force-pushed
the
prototype-scoped-metacache-ut
branch
from
August 11, 2026 10:13
04e684b to
97ef95e
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review complete for head 97ef95eaa15a40ce82eba7f02877a35ea4602f88: no new actionable findings. This is a comment-only, no-blocker opinion.
Critical checkpoint conclusions:
- Goal and proof: the PR unifies FE and connector metadata caches behind catalog-owned runtimes with shared configuration, hierarchical invalidation, lifecycle, and statistics. The implementation accomplishes that goal, and the added unit suites cover load/publication, exact and ancestor invalidation, refresh, bulk loading, close, pruning/leak behavior, routing, and connector migrations.
- Scope and design: this is necessarily broad, but the changes stay focused on the cache-framework consolidation, its compatibility adapters, connector migrations, and their tests. Existing mechanisms are reused and no unrelated behavior change was found.
- Concurrency and locking: query/event/refresh threads, FE stripes, phase gates, key monitors, scope/key generations, Caffeine removal callbacks, and refresh executors were traced. Remote loads and other heavy work remain outside publication locks; reachable production paths have consistent lock ordering; exact wrapper/state checks fence stale load, refresh, CAS, and bulk publication; and callbacks are deferred outside gates. No reachable deadlock or stale-publication defect remains.
- Lifecycle and configuration: connector-owned runtimes close before their remote clients/catalogs, Hive closes built siblings, FE engine runtimes are removed and closed, and key/scope/tombstone state is reclaimed on eviction, invalidation, handle release, and close. Legacy enable/TTL/capacity parsing, disabled behavior, plugin overlays, and rebuild-on-property-change semantics are preserved; no new dynamic configuration contract was introduced.
- Compatibility and parallel paths: default versus Remote Doris routing, the
external_dorisalias, local/remote name mappings, missing-catalog cleanup, replay/DDL/event paths, and ADBC, Hive/HMS/Hudi, Iceberg, MaxCompute, and Paimon scope projections were checked. Snapshot, branch, system-table, namespace, and partition variants converge on their owning ancestor invalidations. No storage format, public symbol, persisted-state encoding, or FE/BE protocol compatibility change is involved. - Tests and results: 24 changed unit-test files include deterministic latch/barrier coverage for the altered concurrency boundaries plus hierarchy, leak, configuration, routing, and connector invalidation cases. No generated regression result changes apply. Builds and tests were not run because the authoritative review instructions explicitly prohibit them, so this conclusion is based on static review of the current head and test sources.
- Error handling, memory, observability, and performance: loader failures/null misses, disabled-cache pass-through, refresh rejection, callback failures, metrics/statistics, bounded hierarchy traversal, single-flight admission, and cleanup accounting were reviewed. Failures are surfaced or logged at the existing boundaries, remote I/O is not performed under cache locks, and no additional material correctness, leak, observability, or performance issue was substantiated.
- Persistence, transactions, data writes, and FE/BE variables: not applicable; this refactor does not alter transaction or user-data write paths, EditLog formats, storage visibility, or variables sent between FE and BE.
The five existing live inline findings—cold object/ID publication, name-snapshot remapping, expected-wrapper refresh replacement, cold descendant batch invalidation, and identity-preserving CAS removal—are addressed in the current head and were not duplicated. There was no additional user-provided review focus.
Review status: converged after two complete full-scope rounds and independent risk-focused rechecks. All 85 authoritative changed paths were swept, every candidate was validated or dismissed with concrete production-path evidence, and no unresolved point or inline comment remains.
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Aug 20, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66634 Problem Summary: Routine Load cannot safely use the legacy Expr object graph as an image or journal compatibility surface. Keep the current effective load definition in origStmt, remove the duplicate execMemLimit JSON source, and cover all SQL-representable load clauses through CREATE image restore, ALTER merge, and a second image restore. ### Release note Routine Load now persists ALTERed load clauses in the effective origin SQL used during FE recovery. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.load.routineload.RoutineLoadJobPersistenceTest - ./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest - Docker regression case added but not run locally - Behavior changed: Yes, ALTERed Routine Load definitions survive journal replay and image recovery - Does this need documentation: Yes, the existing design document and PR description must be updated
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Aug 20, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66634 Problem Summary: Routine Load images already persist and replay origStmt. The persistence bug is that ALTER load clauses changed runtime fields without updating that statement. Keep the existing gsonPostProcess recovery path unchanged, persist the original ALTER SQL in the journal, and rewrite origStmt to a complete effective CREATE statement after leader and follower ALTER application. Remove direct-field persistence, cache hydration, CSV validation, and other adjacent changes from this PR. ### Release note Routine Load now preserves ALTERed load clauses across follower replay, checkpoints, and FE restart by maintaining the effective CREATE statement. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.load.routineload.RoutineLoadJobPersistenceTest - ./run-fe-ut.sh --run org.apache.doris.load.routineload.KinesisRoutineLoadJobTest - KafkaRoutineLoadJobTest and AlterRoutineLoadOperationLogTest passed in the combined targeted run - Docker regression case added but not run locally - Behavior changed: Yes, ALTERed load clauses update the persisted origin statement - Does this need documentation: Yes, document mixed-version ALTER limitations
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Aug 20, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66634 Problem Summary: Treat the effective Routine Load fields as authoritative metadata instead of rewriting origStmt after ALTER. Persist the load-definition fields directly in images, persist RoutineLoadDesc deltas in ALTER journals, and use the original CREATE statement only when reading legacy images whose nullable effective fields are absent. Empty new definitions may also use the fallback safely because ALTER cannot unset all load clauses. ### Release note Routine Load now preserves ALTERed load clauses across journal replay and FE restart through direct metadata persistence. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.load.routineload.RoutineLoadJobPersistenceTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest - Behavior changed: Yes, image and ALTER journal persist effective Routine Load definitions directly - Does this need documentation: Yes, document mixed-version ALTER limitations
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Aug 20, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66634 Problem Summary: Metadata consumers now persist legacy Expr objects directly, but the existing Expr Gson test only checked subtype and JSON idempotence. Add stable serialization for SQL-relevant fields that were silently dropped, persist function ORDER BY metadata, require every Expr instance field to be serialized or explicitly classified as non-durable, and verify SQL output with and without table names for every concrete registered subtype. Add an analysis review guide for future Expr changes. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.analysis.ExprGsonSerializationTest - ./run-fe-ut.sh --run org.apache.doris.analysis.ExprGsonSerializationTest,org.apache.doris.load.routineload.RoutineLoadJobPersistenceTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest - Behavior changed: No user-facing SQL behavior; metadata Expr round trips now preserve SQL semantics - Does this need documentation: No, contributor guidance is included in analysis/AGENTS.md
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Aug 20, 2026
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66634 Problem Summary: The first restoration of direct Routine Load persistence omitted parts of the previously reviewed design. Restore the exact persistence implementation from commit 4394fa3, including execMemLimit and memtableOnSinkNode image fields, jobProperties cache hydration, CSV ALTER cache synchronization, leader-only validation, legacy image migration, and the original Kafka/Kinesis persistence tests. Keep the separate Expr serde hardening on top. ### Release note Routine Load persists its effective load definition and non-default task configuration directly across FE recovery. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.analysis.ExprGsonSerializationTest,org.apache.doris.load.routineload.RoutineLoadJobPersistenceTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest - Behavior changed: Yes, restore the complete direct-state image and ALTER journal persistence contract - Does this need documentation: Yes, document mixed-version ALTER limitations
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Aug 24, 2026
Problem Summary: PR apache#64160 added row-count cache invalidation and a test-only engine cache replacement hook after this branch diverged. Merging current master conflicted with the scoped metadata cache test setup and left the old registry-based test hook incompatible with the new cacheTypes framework. Solution: Preserve the row-count cache implementation and invalidation ordering from apache#64160. Resolve RefreshManagerTest against FeMetaCacheEntry and migrate replaceEngineCachesForTest to register replacement engines and aliases through the new cacheTypes model. Tests: - 116 focused FE unit tests passed - 104 connector cache unit tests passed - Maven Checkstyle reactor passed with 0 violations Issue Number: close apache#66633 Related PR: apache#64160
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Aug 24, 2026
Problem Summary: Master advanced after the initial conflict resolution and introduced Paimon HMS identity isolation changes that overlapped the scoped Paimon cache refactor at the connector imports. Solution: Preserve the new HMS FileIO and catalog construction path while retaining CatalogMetaCache-based invalidation. Drop only the obsolete Identifier import, which is no longer used after table invalidation is routed through the scoped cache hierarchy. Tests: - 554 Paimon connector tests passed (1 skipped) - Checkstyle validation passed in the FE reactor Issue Number: close apache#66633
Contributor
Author
|
run buildall |
Contributor
TPC-DS: Total hot run time: 82249 ms |
Contributor
ClickBench: Total hot run time: 14.66 s |
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66914 Problem Summary: The unified cache runtime used the removal callback for values that never became cache-owned and acquired refresh publication leases before asynchronous tasks started. A disabled or invalidation-rejected FE database load could therefore return an already-reset object, identity refresh could retire the same object, and discarded queued refresh work could retain publication state. Separate unpublished-value discard from published-value removal, keep identity refresh wrapper ownership, acquire refresh leases inside tasks, release refresh admission on close, and remove a stale external-table comment. ### Release note None ### Check List (For Author) - Test: Unit Test - fe-connector-cache module tests (110 tests) - FeMetaCacheEntryTest (14 tests) - IcebergTableCacheTest (12 tests) - Full FE build - Behavior changed: No. Preserves pre-refactor ownership and cleanup semantics. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review result: request changes. I found one new actionable FE publication race and attached it inline. I read the existing review threads and raw comments first; no existing issue is duplicated. There was no additional user-provided focus.
Checkpoint conclusions:
- Goal, accomplishment, and tests: The change establishes the shared catalog-scoped metadata-cache framework and migrates the intended FE/connector caches with substantial focused unit coverage. MAIN-01 shows that the FE name/object/ID publication goal is not yet complete for incremental database registration.
- Scope and clarity: The diff is large but cohesive around one cache-framework migration. Framework, FE adapter, and connector-specific responsibilities are generally separated clearly.
- Concurrency: I traced miss election, refresh, direct put/CAS, bulk publication, exact and hierarchical invalidation, pruning, synchronous callbacks, and close across the phase gates, per-key monitors, and FE stripes. The inline issue is a distinct unlocked ID-index visibility window before the object CAS. I found no additional lock inversion, stale-publication, leak, or double-retirement schedule beyond the already-live threads.
- Lifecycle and initialization: Shared owners are closed by each migrated connector; late loads are fenced, and Iceberg cache/loader/borrower/catalog-generation references remain balanced. Static initialization and connector classloader boundaries did not expose a new issue.
- Configuration: Enable/disable, TTL, refresh, capacity, stripe-count, and connector-specific cache knobs retain their intended construction-time behavior. No new dynamic-config handling defect was found.
- Compatibility and rolling behavior: The moved cache API is not a connector SPI contract; plugin dependency/classloader closure and current call sites were checked. Remote Doris/default routing and exact built-in engine constants remain reachable. No wire, persisted-format, or mixed-FE/BE compatibility change was introduced.
- Parallel paths: ADBC, Hive/HMS/Hudi, Iceberg, MaxCompute, Paimon, FE database/table caches, and missing-catalog cleanup were compared. Scope/invalidation parity is preserved; the one surviving discrepancy is the incremental FE database path described inline.
- Special conditions and related paths: Cold/hot entries, disabled caches, rejected publication, eviction, refresh, batch invalidation, catalog/database/table/partition invalidation, close, and late completion were checked. Similar tests and callers were inspected, and comments match the current mechanisms.
- Test coverage and modularity: The framework has focused hierarchy, concurrency, leak, lifecycle, and connector tests. The missing negative case is a latch test that lets a by-ID database lookup publish between
validation.run()and the CAS. No local build or test was run because this review task explicitly prohibited it. - Test results: At review time, BE UT, Cloud UT, coverage, dependency review, style, license, and secret checks passed; compile, FE UT, performance, and the code-review workflow were still pending. I did not infer success for pending checks.
- Observability: Cache hit/miss/load/eviction/error and hierarchy metrics remain available; refresh/close failures retain logging. No new observability blocker was found.
- Transactions and persistence: This refactor does not change transaction boundaries, edit logs, durable metadata formats, or failover replay semantics.
- Data writes and crash safety: The change affects ephemeral metadata caching rather than storage-engine writes, so atomic durable-write/crash-recovery checks are not applicable.
- FE/BE variables and nullable semantics: The changed behavior is FE/connector-local and adds no FE-to-BE variable or protocol path. BE nullable-column handling is not applicable.
- Performance: Remote loaders remain outside critical sections, shared scopes are bounded/pruned, and cache capacities remain explicit. I found no distinct new performance regression.
- Other issues: No additional valuable findings survived validation and duplicate fencing.
Contributor
TPC-H: Total hot run time: 16978 ms |
Contributor
TPC-DS: Total hot run time: 81459 ms |
Contributor
ClickBench: Total hot run time: 14.64 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
### What problem does this PR solve? Issue Number: close apache#64159 Related PR: apache#64160 Problem Summary: Incremental metadata events published the ID-to-name mapping before the matching database or table object was committed to the shared cache. A concurrent lookup by ID could therefore load and publish a different object in that gap, which was then replaced and cleaned up by the event update. Publish the ID mapping and object inside one guarded cache commit, fence overlapping loads, and make the object-and-ID step run only once when the outer name-cache CAS retries so a newer object is never replaced by replaying an already completed event update. ### Release note None ### Check List (For Author) - Test: Unit Test - ScopedMetaCacheConcurrencyTest - FeMetaCacheEntryTest - ExternalCatalogTest - ExternalDatabaseTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
924060929
force-pushed
the
prototype-scoped-metacache-ut
branch
from
August 27, 2026 07:55
529ed2d to
f04e5f3
Compare
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review result: NO_NEW_VALUABLE_FINDINGS at f04e5f3f8cab42a6ac3f18a5f10e19a7aa028e82.
Checkpoint conclusions:
- Goal and proof: The current head unifies FE and connector metadata caches behind one catalog-scoped runtime while preserving scoped invalidation, publication fencing, cleanup, and connector-specific ownership. The changed unit tests exercise the shared hierarchy/concurrency contracts and the FE/connector adapters.
- Scope and focus: The 88-file change is large but cohesive: shared runtime, all affected connector migrations, FE adaptation/routing, dependencies, and their tests. No unrelated behavior change was found. There was no additional user-provided review focus.
- Concurrency: Query misses, asynchronous refresh, invalidation/event threads, eviction callbacks, and catalog close can race. The reviewed order is bulk phase gate -> registry publication gate -> key state/monitor, with FE name/object stripes around their local publication actions. Remote loads and physical removal cleanup stay outside the publication locks; no distinct remaining deadlock, stale-publication, or cleanup leak was substantiated.
- Lifecycle and ownership: Per-catalog owners close their cache registries; queued refresh owns no pre-run lease; invalidation/close fence later publication; FE removal and discard ownership are separated; and Iceberg table/cache/borrower references drain before resource close. No circular or unbounded retained ownership was found.
- Configuration: No new global dynamic configuration is introduced. Existing enable/TTL/capacity semantics and legacy Hive/HMS property translations are retained, with new-key precedence. Cache specs are captured at catalog/runtime construction as before.
- Compatibility and rolling upgrade: No storage, journal, RPC, FE-BE protocol, or serialized metadata format changes are present. Legacy catalog properties are translated, and the shared cache API/runtime is present in the FE and connector dependency/classloader paths.
- Parallel paths: ADBC, Hive/HMS/Hudi, Iceberg, MaxCompute, Paimon, generic external catalogs, and Remote Doris routing were checked. Local/remote identity domains, sibling invalidation, partition collection/value scopes, and Iceberg's catalog-scoped manifest exception remain aligned with their callers.
- Conditions and error paths: Disabled caches, null/failed loads, refresh rejection, direct put/CAS, bulk publication, exact and hierarchical invalidation, dropped catalogs, alias routing, callback failures, and close races were reviewed. The special branches have matching contracts/tests or preserve the previous behavior.
- Test coverage: The changed tests cover hierarchy, phase gates, miss/refresh/invalidation races, callback reentrancy, pruning/leak checks, FE database/table publication ordering, connector invalidation, and Iceberg resource ownership. No SQL regression or end-to-end result files are added; this is an internal FE/connector cache refactor.
- Test results: Source assertions and expected outcomes were reviewed and are consistent with the intended contracts. Per the authoritative review prompt, I did not run builds or tests; the PR description reports the author's unit-test and FE-build results.
- Observability: Cache metrics retain hits, misses, loads, latency, evictions, invalidations, size, and last-error timestamps; failure logging includes the relevant cache/catalog context. No additional distributed tracing surface is needed for this internal runtime.
- Transactions and persistence: No transaction, EditLog, replay format, or master-failover protocol is modified.
- Data writes and atomicity: No user-data write path is changed. Metadata publication/invalidation uses identity-conditional replacement and logical fencing before physical cleanup; no new crash-time persistence obligation is introduced.
- FE-BE variables: No variable, thrift field, planner payload, or other FE-BE transmission path is added or changed.
- Performance: Misses remain deduplicated, refresh remains asynchronous, caches stay bounded by TTL/capacity, and hot-path invalidation uses scoped registration instead of full predicate scans. Lock-held work is limited to publication state; no new material CPU, allocation, or memory-growth issue was substantiated.
- Other issues: Existing review threads and all raw review comments were used as duplicate fences. After the main audit plus normal-runtime, integration, and risk-focused second passes, every candidate was either already covered or dismissed with current/base code evidence; no new inline issue remains.
Contributor
TPC-H: Total hot run time: 16856 ms |
Contributor
TPC-DS: Total hot run time: 82003 ms |
Contributor
ClickBench: Total hot run time: 14.48 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
approved these changes
Aug 31, 2026
CalvinKirs
approved these changes
Aug 31, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
4 participants
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: None
Related PR: #66407
Problem Summary:
FE naming caches and connector metadata caches currently use duplicate cache wrappers and independently maintained invalidation dependencies. Each connector must remember every sibling cache affected by catalog, database, table, or partition invalidation. Concurrent loads, refresh, eviction callbacks, and catalog close also need consistent publication and cleanup semantics across these implementations.
This PR makes
fe-connector-cachethe shared metadata-cache runtime while preserving one cache owner per catalog. It introduces declarative cache definitions and hierarchical scopes, then migrates ADBC, Hive, HMS, Hudi, Iceberg, MaxCompute, and Paimon caches to the shared framework. FE keeps a thin adapter for Doris-specific naming, object, andIdNameIndexpublication semantics.The shared runtime provides:
The change also removes duplicate
CacheSpec,CacheFactory, and legacy connector-cache entry implementations. FE-specific cache publication retains the lock ordernames -> object -> IdNameIndex, and validates identity before mutating cache state. After synchronizing the latest master, Iceberg's snapshot-scoped equality-delete field-ID cache is also managed by the same per-catalog owner, so catalog invalidation and close cover both manifest and equality-delete metadata.Performance measurements on the final commit show approximately 85.8 ns/op for the public
CatalogMetaCache -> MetaCachehit path. Eight-thread publication reached 2.85M ops/s for one cache and 2.51M ops/s for separate caches in one registry. Invalidating and physically cleaning 120,000 entries took approximately 32 ms on the local benchmark.Release note
None
Check List (For Author)
Test
fe-connector-cache: 100 tests passedIcebergScanPlanProviderTestandIcebergManifestCacheTest)./build.sh --fe: all 73 Maven reactor modules passed, including Checkstyle; the outer script subsequently returned non-zero while assembling already-built filesystem plugin archives intooutput/Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)