[refactor](catalog) Unify external metadata cache framework - #66633
Open
924060929 wants to merge 10 commits into
Open
[refactor](catalog) Unify external metadata cache framework#66633924060929 wants to merge 10 commits into
924060929 wants to merge 10 commits into
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: 82402 ms |
Contributor
ClickBench: Total hot run time: 14.87 s |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Metadata cache invalidation needs catalog, database, table, partition, and exact-key isolation without stale publication races or unbounded secondary-index retention. Add an isolated catalog-local scoped cache prototype with generation-aware single-flight loading, exact-key and hierarchical publication fencing, atomic bulk commit, conditional physical cleanup, and bottom-up scope/key-node reclamation. The prototype is not wired into existing metadata cache callers yet. Add deterministic tests for the complete scope matrix, refresh/load/bulk/close races, delayed removal callbacks, generation overflow, eviction, lifecycle closure, high-cardinality churn, and a seeded reference-model state machine.
### Release note
None
### Check List (For Author)
- Test: Unit Test and FE build
- Unit Test: fe-connector-cache Maven reactor, 86 tests passed
- Unit Test: focused JDK 17 run-fe-ut review suite passed
- FE build: JDK 17 ./build.sh --fe with a fresh output directory passed
- Behavior changed: No. The prototype is not connected to production callers.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The scoped metadata cache correctness prototype allocated generic lists and snapshots on scope acquisition, recomputed single-flight address hashes, serialized bulk candidates through a redundant preliminary lock, and performed duplicate detached-entry cleanup. Share an immutable fixed-layout generation snapshot per leaf state, cache address hashes, publish bulk values only after the final fence succeeds, and make invalidation own each detached registration before physical removal. In local microbenchmarks this reduces cache-hit latency from 84-95 ns to about 61 ns, reduces same-table retained overhead from about 569 bytes to 369 bytes per entry, and raises eight-thread bulk throughput from about 1.33 million to 1.56 million operations per second while preserving the state-identity invalidation protocol.
### Release note
None
### Check List (For Author)
- Test: Unit Test and FE build
- Unit Test: fe-connector-cache Maven reactor, 87 tests passed
- Unit Test: focused JDK 17 run-fe-ut review suite, 54 tests passed
- FE build: JDK 17 ./build.sh --fe with a fresh output directory passed
- Behavior changed: No. The prototype is not connected to production callers.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Cold scoped metadata loads repeatedly allocated varargs arrays and capturing lambdas and computed identity hashes for state tokens even though single-flight hashing only needs stable key and path distribution. Use allocation-free path hashing cached per shared scope snapshot, use a get/putIfAbsent scope-child fast path, cache cache-address hashes, keep full generation identity comparisons in LoadAddress.equals, and remove a redundant bulk scope check before the final publication fence. Matched local microbenchmarks reduced cold load from 652.5-768.5 ns/op to 442.6 ns/op while independent cache-hit latency remained effectively unchanged. Current-head profiling no longer shows the identityHashCode, ScopePath Object array, or child lambda hotspots.
### Release note
None
### Check List (For Author)
- Test: Unit Test and FE build
- Unit Test: 87 fe-connector-cache tests and 54 focused scoped-cache tests
- FE build: ./build.sh --fe
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Scoped metadata bulk publication used exclusive per-cache and registry monitors, serializing independent keys and cache owners. Replace those publication barriers with writer-preferred striped phase gates while keeping exact-key and hierarchical invalidation exclusive. Defer synchronous Caffeine removal callbacks until publication phases are released, preserve expected-value cleanup under callback reentrancy and failures, and coordinate scope-node retain, child creation, invalidation, and pruning with an exact lifecycle marker. In matched eight-thread benchmarks, the correctness-safe implementation improves same-cache publication from about 1.3-1.4 M ops/s to 2.8-3.2 M ops/s and same-registry publication from about 1.8 M ops/s to 3.0-3.3 M ops/s. CPU and lock profiles show the remaining same-cache lock bottleneck is Caffeine maintenance rather than the framework publication barriers.
### Release note
None
### Check List (For Author)
- Test: Unit Test and manual performance profiling
- Unit Test: `./run-fe-ut.sh --run org.apache.doris.connector.cache.*Test` (99 tests; full FE reactor BUILD SUCCESS)
- Manual test: matched eight-thread topology benchmarks and async-profiler CPU/lock profiles
- Behavior changed: Yes (independent bulk publications can overlap while invalidation and close remain exclusive)
- Does this need documentation: No
morrySnow
changed the title
Issue Number: None
Related PR: None
Problem Summary: FE naming caches and connector metadata caches used duplicate cache wrappers and independently maintained invalidation dependencies. This refactor makes fe-connector-cache the shared runtime for scoped generation fencing, hierarchical invalidation, load deduplication, refresh, eviction cleanup, and lifecycle management. FE keeps only its naming and ID-index publication adapter, while ADBC, Hive, HMS, Hudi, Iceberg, MaxCompute, and Paimon use catalog-scoped shared cache owners. It also removes the duplicate CacheSpec and obsolete cache implementations, preserves per-catalog isolation, and closes concurrent identity-publication and refresh-close cleanup windows.
None
- Test: Unit Test
- fe-connector-cache: 100 tests passed
- FE targeted metadata cache tests: 93 tests passed
- Iceberg cache tests: 66 tests passed
- ./build.sh --fe
- Behavior changed: No. This is an internal cache framework refactor preserving existing metadata semantics.
- Does this need documentation: No
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. None - 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
Issue Number: close apache#66633 Problem Summary: Direct, bulk, and loader publication paths could invoke synchronous removal callbacks while publication locks were still held or acquire a key monitor before the phase gates. Route all commits through the phase-gate/key ordering, defer callbacks until after unlocking, and restore the merged row-count cache test seam against the new engine cache registry. None - Test: Unit Test - Scoped metadata cache module and concurrency tests - ExternalRowCountCacheTest and RefreshManagerTest - Paimon and Iceberg connector Maven tests - Full FE build - Behavior changed: No. The change preserves cache publication and invalidation semantics while removing lock inversion and callback re-entry hazards. - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#66633 Related PR: apache#66914 Problem Summary: Rebasing the unified external metadata cache framework over the Iceberg resource-lifecycle fix exposed a missing ownership transition. Values loaded while caching is disabled, or values whose load or refresh loses a concurrent publication race, never become cache-owned but still reserve a cache-owner reference. Retire those unpublished values through the removal callback, preserve the explicit removal reason, and cover disabled loads, rejected refreshes, and stale concurrent loads with regression tests. Also remove stale references to the deleted MetaCacheEntry type. ### Release note None ### Check List (For Author) - Test: Unit Test - fe-connector-cache module tests (108 tests) - IcebergTableCacheTest (12 tests) - Iceberg connector module test suite (1355 tests; one unrelated existing IcebergWritePlanProviderTest failure) - Full FE build - Behavior changed: No. The change preserves the resource-retirement semantics of the merged Iceberg lifecycle fix in the unified cache framework. - Does this need documentation: No
924060929
force-pushed
the
prototype-scoped-metacache-ut
branch
from
August 26, 2026 02:40
eb5efc9 to
1182998
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: four distinct P2 ownership/lifecycle defects remain in the new shared cache runtime.
The overall design is coherent and does centralize connector cache ownership, hierarchical invalidation, metrics, and FE adaptation as intended, with substantial unit coverage. The current implementation does not yet preserve value ownership across disabled loads, invalidation-losing loads, identity-preserving refresh, and executor cancellation.
Critical checkpoint conclusions:
- Concurrency: query misses, refresh-executor work, exact/scope invalidation, and catalog close were traced end to end. The current bulk -> registry -> key lock order and deferred synchronous callbacks close the already-reported deadlocks; the four inline lifecycle schedules remain.
- Lifecycle/resources: disabled and rejected miss results can be reset before their caller receives them; identity refresh can retire the retained object; an accepted but discarded refresh task can permanently retain its lease/marker. Iceberg's explicit temporary loader/borrow references are otherwise balanced.
- Configuration: the existing supported TTL-zero setting deterministically triggers one finding. No separate new-configuration or dynamic-update defect was found.
- Parallel paths/compatibility: ADBC, Hive/HMS, Hudi, Iceberg, MaxCompute, Paimon, FE engine routing, child-first plugin loading, and pom/Caffeine boundaries were checked. Scope identities match their invalidation callers; no FE/BE protocol, storage-format, persistence, transaction, or data-write compatibility issue applies.
- Tests/results: 27 changed test files cover the framework broadly, but omit the four ownership/cancellation schedules called out inline. Local builds/tests were not run because this review runner explicitly forbids builds. Live style, license, secrets, and dependency-review checks pass; FE UT, compile, performance, and this automated review check are still pending.
- Observability/performance: cache metrics and logging are generally adequate. The TTL-zero defect also turns read-only database lookup into repeated downstream metadata invalidation; no additional distinct performance issue survived review.
- User focus: the supplied focus file requested no additional focus.
The bounded three-round review converged with no new finding in the cap round; it is complete rather than capped/incomplete. All candidates were independently verified and deduplicated against the live review threads.
Contributor
TPC-H: Total hot run time: 16937 ms |
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
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
3 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)