Repository navigation
Conversation
### What problem does this PR solve? Issue Number: None Problem Summary: FE external catalog names, lookup objects, and schemas had separate owners. Explicit refresh could retain a dropped table in its membership list, and independent lifecycle handling allowed stale loads or discarded database instances to outlive their cache registrations. Share one catalog invalidation owner while preserving per-database capacity/TTL. Explicit refresh invalidates the target and parent membership together; write-triggered refresh remains metadata-only, including replay. Retire database-instance entries without blocking remote loads, let schema runtimes borrow rather than close the shared owner, and keep cold replay safe after cache reconfiguration. Lookup handles retain count-based admission while names and schema payloads use configured byte budgets. ### Release note Explicit external table and database refresh now invalidates cached lookup objects and parent membership consistently. Ordinary write-triggered metadata refresh keeps cached table names. ### Check List (For Author) - Test: Unit Test (290 related tests passed); Regression test (Hive metadata refresh against an isolated FE and real HMS); FE build and Checkstyle passed. No multi-FE cluster, actual Trino, or BE data execution validation. - Behavior changed: Yes (explicit refresh also invalidates lookup and parent membership; ordinary write-triggered refresh remains metadata-only) - Does this need documentation: No (existing refresh commands and retention policy; cache-definition API documents byte-accounting boundaries)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: three verified inline findings (one P1, two P2). Catalog rename/replay permanently retires the reused catalog instance; disabled database caching with case-insensitive table names breaks DROP/RENAME event application; existing refresh tests retain objects whose replacement lifetimes invalidate their timestamp assertions.
Reviewed all 20 authoritative changed paths at ae04d1e, including related callers, replay, connectors and tests. There were no existing inline threads or blocking IDs and no additional user-provided focus. All three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final comment set in round 2. Every candidate and initial risk has an evidence-based disposition; no static scope gap remains.
Critical checkpoint conclusions:
- Goal and correctness: the shared owner connects identity, membership and schema invalidation; ordinary write-triggered refresh preserves lookup/name caches. The rename and retired-database findings prevent fully correct integration. Internal row visibility, visible versions and delete bitmaps are unchanged.
- Scope and reuse: the change stays focused on FE cache ownership, refresh behavior and supporting tests, using the existing cache framework.
- Concurrency and locks: traced query workers, commonRefreshExecutor, refresh, events, replay and ALTER. Cache publication uses adapter stripes, scope generations and key-node locks; remote cache loaders run outside publication locks and removal callbacks are deferred. No separate deadlock or stale-publication defect was substantiated.
- Lifecycle: exact-registration removal and borrowed schema runtimes preserve replacement entries. Terminal retirement is incorrectly reused by rename, and never-initialized retired database handles expose the event-path failure. No C++ static-initialization concern applies.
- Configuration: no new user setting. Existing count/TTL/stripe and payload-byte settings remain; ALTER retirement/reconfiguration was traced. TTL-zero operation is the verified conditional failure.
- Compatibility: the Gson-annotated refresh flag defaults false in old records, while new explicit refresh sets true. No function ABI, data-storage format or connector-SPI module change. Rename replay shares the verified retirement regression.
- Thrift/protobuf: no changed definitions or field IDs.
- Parallel paths: inspected default/Remote Doris, explicit/mutation/event/partition/rename refresh, hot/cold replay, system databases, name-case modes and session bypass. Metadata-only writes retain their intended scope.
- Conditions and errors: synthetic scope keys avoid user-name collisions; admission and retirement branches were checked. Missing plugin table handles now raise a named CacheException with scope cleanup in finally. The retired mode-2 unregister branch needs correction.
- Tests: new latch-based tests cover blocked loads, recreation, retirement, reconfiguration, budgets, replay and statistics. The Hive regression exercises independent catalogs. Rename and disabled-cache event cases need coverage, and both existing refresh timestamp tests need adaptation as detailed inline.
- Expected results: regression listing order, error assertions and int/text/bigint schema outputs match the scripted operations. Output generation and the author's reported test runs were not independently reproduced.
- Observability: unified statistics include naming-only owners and deduplicate engine/name rows; existing refresh and event logs identify the affected objects. No further observability defect was found.
- Persistence/failover: refresh EditLog writes and matching replay dispatches are paired, including explicit lookup scope and legacy defaults. Cold replay remains cache-only. Rename replay is affected by the P1 finding.
- Data writes and crash behavior: no transaction or data-write protocol is changed. Committed external-mutation logs still precede fallible local invalidation; cache generations and exact cleanup fence discarded loads. No additional crash-related leak was demonstrated.
- FE/BE passing: no new transmitted variables or BE paths.
- Performance and ownership: COUNT_ONLY preserves lookup count/TTL retention without traversing mutable catalog backreferences; names/schema remain byte-budgeted. No additional substantiated CPU, memory or complexity issue remained after the missed-path sweeps.
Validation limits: static source review only, as required. No builds, tests, runtime reproduction, multi-FE execution or performance measurements were performed. Repository source was not modified.
Review converged after 2 round(s).
| @Override | ||
| public final void onClose() { | ||
| synchronized (this) { | ||
| feCacheRetired = true; |
There was a problem hiding this comment.
[P1] Allow renamed catalogs to initialize a new cache owner
CatalogMgr.alterCatalogName() calls cleanupRemovedCatalog() (and therefore onClose()), then renames and re-registers the same catalog instance. Setting this flag permanently means ALTER CATALOG old RENAME new succeeds, but every subsequent metadata access fails in getFeMetaCacheOwner() with "External catalog cache owner has been retired". resetToUninitialized() never clears the flag, and replayAlterCatalogName() reuses the instance in the same way, so followers are affected too. Distinguish terminal removal from rename cleanup, or explicitly reactivate/recreate the owner when re-registering the catalog; cover rename and its replay.
| localTableName = resolveTableNameForInvalidation(tableName); | ||
| // Always clean local names/object/ID state, even when the object entry is cold or already evicted. | ||
| invalidateTableCache(localTableName); | ||
| if (!cacheEntriesReleased) { |
There was a problem hiding this comment.
[P2] Skip cache-based name resolution for retired databases
With external_cache_expire_time_seconds_after_access=0 and lower_case_table_names=2, the database lookup returns an uncached database and its new discard callback retires it before tables is initialized. An HMS DROP/RENAME event reaches unregisterTable(), but makeSureInitialized() now returns immediately for that database and resolveTableNameForInvalidation() still runs before this guard. Its case-insensitive fallback dereferences tables.findIfPresent() on null. Event application then fails and the sync driver resets its cursor for a full resync. Include name resolution in the retired-entry handling (or provide a transient resolver), and cover this disabled-cache configuration.
| try { | ||
| db.resetMetaToUninitialized(); | ||
| } finally { | ||
| ((ExternalCatalog) db.getCatalog()).invalidateDatabaseLookup(db.getFullName()); |
There was a problem hiding this comment.
[P2] Update the existing refresh tests for replacement objects
Lookup eviction changes the object lifetime, but RefreshDbTest.testRefreshDatabase() still retains db1 across refresh and asserts that its lastUpdateTime later increases. That database is now permanently retired, so subsequent live reads leave its timestamp unchanged and the l4 > l3 assertion fails. Likewise, RefreshTableTest.testRefreshTable() retains the evicted table and expects getFullSchema() to advance its timestamp; the schema loader now resolves and updates the replacement table instead. Update both existing tests to reacquire the current objects and assert the intended replacement behavior so the FE suite remains compatible with this change.
### What problem does this PR solve?
Issue Number: None
Problem Summary:
External catalog metadata is cached in several dependent layers: database/table membership, FE object and ID lookup, schemas, row counts, and connector metadata such as partitions, files, and snapshots. Previously these layers had separate owners and callers manually invalidated both sides. A refresh could leave a stale membership or object lookup behind; a load started before invalidation could republish an obsolete object or identity, particularly after a drop and same-name recreation. Keeping the old FE database object after eviction also allowed its cleanup to affect a replacement.
Use one `CatalogMetaCache` root per external catalog. FE registrations and instance-owned connector domains share its resource generations and existing budgets. Embedded connectors use child domains. Retiring an instance removes only its exact registrations; semantic resource invalidation is independent of instance lifetime. Local names, remote names, and numeric derived-state identities use distinct coordinate spaces.
`CatalogMetadataChanges` is the common boundary for refresh, successful writes/DDL, metastore events, and replay. It projects affected local and remote resources and invalidates them at one publication barrier. Connectors describe cross-resource dependencies through a pure SPI projection instead of imperative sibling invalidation. DDL journal records preserve remote identities for replay; old records fall back to broader remote-only invalidation without initializing remote metadata. Content changes preserve table-name snapshots and FE lookups, while explicit table/database refresh also invalidates the appropriate membership snapshot.
ID mappings survive ordinary object eviction but participate in semantic invalidation. Publication is guarded by the same resource generation, and accepted operations pin their auxiliary index until completion. Row-count futures belong to catalog roots while a shared retention-policy index preserves the existing process-wide entry limit. Cache callbacks retain the owning plugin classloader context.
Automatic row-count refresh keeps the last successful value while reloading and on failure. ADBC membership caches register under the shared collection scopes.
This also addresses catalog rename/replay reactivation, disabled database caching with case-insensitive table lookup, and tests that continued to use retired database instances. The connector plugin API major version changes from 19 to 20 with updated surface coverage.
### Release note
Fix stale external-catalog table membership and metadata after refresh, including drop and same-name recreation. Ordinary data writes keep cached table-name listings.
### Check List (For Author)
- Test: Unit Test / Regression test
- Selected connector modules and upstream dependencies packaged with their own tests, including SPI, Hive, Hudi, Iceberg, Paimon, Fluss, ADBC, and MaxCompute.
- Current cache framework suite: 172 tests passed; connector SPI own suite: 82 tests passed.
- Related FE coverage: 320 unique tests. A broad 317-test run had one obsolete dynamic-proxy error; after fixing its null collection return, the affected class and cache-entry lifecycle tests passed all 24 tests, including the added retirement concurrency case. The independent Astra review found two additional gaps; after reproducing both on the old implementation, the fixes passed 40 targeted tests (16 row-count and 24 ADBC tests), including successful/failed automatic refresh and root-level membership invalidation.
- Full FE build and Checkstyle passed using `./build.sh --fe -j4` with JDK 17.
- `test_hive_catalog_owner_refresh` passed against the newly packaged isolated FE and real HMS: 1 suite, zero failed/fatal/skipped, comparing the checked-in expected output.
- No multi-FE cluster, actual Trino service, or BE data-execution validation.
- Behavior changed: Yes. Explicit refresh invalidates dependent membership and metadata together; content-only changes preserve names and object lookup. Connector plugin API major version is 20.
- Does this need documentation: No. Internal ownership and SPI integration are documented in `fe/fe-connector/README.md`.
924060929
changed the title
|
run buildall |
|
/review |
There was a problem hiding this comment.
No new actionable findings at head 328deaa948135f3b4a2798e366e87dae566835d1. Static review covered all 78 changed paths (32 connector, 44 FE core, 2 regression), related callers, and the existing inline threads. Two complete-scope reviewers and an additional publication/lifecycle risk reviewer all returned NO_NEW_VALUABLE_FINDINGS in round 1; main-agent verification found no unresolved candidates. The review converged.
Existing thread dispositions: 4346749470 is addressed by registry reactivation through onRegistered() and replacement cache-owner creation for rename/replay. 4346749481 is addressed by the retired/uninitialized database guard before cache-based name resolution. 4346749491 is addressed by reacquiring replacement database/table objects in the refresh tests. None remains an independently confirmed P0/P1 blocker, and none is reposted.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | Shared catalog ownership, separate instance retirement, and coordinated LOCAL/REMOTE/ID invalidation implement the stated cache-consistency goal. Added domain, FE lifecycle, replay, and Hive regression coverage matches that behavior. |
| Scope and reuse | The change is substantial but focused on the ownership/invalidation migration and reuses typed cache definitions and journal carriers. |
| Concurrency and locking | Traced query loads, refresh/DDL, event/replay workers, asynchronous refresh/removal, and retirement. Nested shared reads avoid the suspected gate/index cycle; identity invalidation uses its entry-local gate. Remote loading stays outside publication guards. No reachable new deadlock was verified. |
| Lifecycle | Exact registrations and identity tokens isolate old instances from replacements; accepted table operations pin auxiliary cleanup. Closed roots and retired databases have explicit replacement/transient paths. No cross-TU initialization applies. |
| Configuration | No new configuration keys. Existing TTL/count/weight settings and ALTER retirement/rebuild paths remain in use; schema-only reconfiguration borrows the root without retiring siblings. |
| Compatibility | SPI major version changes from 19.0 to 20.0 with the plugin surface baseline updated. Added Gson journal fields retain legacy defaults and cache-only fallback. No storage or FE/BE wire format change was identified. |
| Thrift/protobuf | No changed definitions; field-ID checks are inapplicable. |
| Parallel paths | Checked explicit/content/partition refresh, create/drop/rename/truncate, events, mapped/case-insensitive names, warm/cold replay, generic/remote-Doris schema routing, and connector siblings. |
| Conditional checks | Closed/retired states, disabled retention, lookup versus content changes, and unknown remote identities have concrete lifecycle/configuration cases and focused tests. |
| Test coverage | Inspected unit coverage for publication races, retirement, sibling preservation, TCCL, identity invalidation, row-count retry/refresh/global retention, and replay. Hive regression covers membership and schema across drop, database refresh, and recreation. |
| Changed results | Expected sorted names and INT/text/BIGINT schemas match the regression SQL sequence. Result-generation provenance cannot be established by static inspection. |
| Observability | Existing refresh/error logs and cache metrics remain; catalog statistics enumerate shared registrations. No concrete additional observability defect was verified. |
| Persistence and failover | Matching write/replay paths preserve remote identities before fallible post-commit bookkeeping; legacy replay widens only remote invalidation without loading remote metadata. |
| Writes and atomicity | Connector transaction behavior is unchanged; post-commit invalidation uses one root publication barrier across dependent cache coordinates. |
| FE/BE propagation | No new transmitted variable or plan field. |
| Performance | Content changes retain names/lookups, cleanup removes exact registrations, and row counts retain a process-wide count bound. No new remote I/O in cache projection/replay or substantiated performance regression was found. |
| Other issues and user focus | No additional substantiated issue. No additional user-provided focus was supplied. |
Limits: this was a static-only review. No builds or tests were run and no source files were modified. Runtime, multi-FE failover, external-service, and BE execution behavior were not independently validated. No changed-path coverage gap or unresolved candidate remains within the authorized review scope.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28370 ms |
TPC-DS: Total hot run time: 150805 ms |
ClickBench: Total hot run time: 24.33 s |
### What problem does this PR solve? Issue Number: None Problem Summary: Full CI exposed four FE test assumptions left behind by the unified metadata cache lifecycle: missing connector handles now fail explicitly, initialization constructs an empty shared cache owner, and the deadlock fixture needs real catalog properties. Update the assertions and fixture while retaining the per-session isolation and lock-order checks. Database DROP now invalidates membership immediately, so the nested Iceberg namespace regression must expect live inferred parents rather than a stale pre-refresh list. Regenerate its expected output with the regression runner. ### Release note None ### Check List (For Author) - Test: Unit Test (36 related FE tests passed); Checkstyle (zero violations); Regression test (complete iceberg_and_internal_nested_namespace suite generated and then compared successfully against the packaged FE and real REST/MinIO). The temporary native ARM runtime used a cached 4.1 BE with be_exec_version=13; no new BE build or full CI success is claimed. - Behavior changed: No. Tests follow the already implemented cache semantics. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of head 450da63290057dac1995f90561fdaa97e152e64d: one new P2 finding, posted inline. Rename must also invalidate a case-equivalent cached destination and its numeric identity when lower_case_table_names=2; otherwise by-ID statistics and constraint readers can retain the dropped destination.
All 84 changed paths were reviewed by the main agent and assigned coverage reviewers, with a separate reviewer for publication/retirement risks. The final frozen inline set was checked in the second round. No additional user focus was supplied.
The three prior threads are fixed on this head and are not duplicated: catalog rename/replay reactivates the registered instance (4346749470); retired/uninitialized database name resolution is guarded (4346749481); refresh tests reacquire replacement objects (4346749491). No previously reported P0/P1 was independently confirmed to remain applicable.
The reviewed flow is SQL refresh / successful external DDL / metastore event / follower replay -> cache-only identity projection -> shared catalog publication barrier -> LOCAL, REMOTE and numeric-ID invalidation. Connector construction acquires instance domains; embedded connectors acquire child domains; physical retirement removes exact registrations.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proof | Unified ownership and the distinction between content mutation and lookup refresh are implemented with substantial owner, cache, replay and regression coverage. The canonical rename-destination case has the inline P2 gap. Tests were inspected, not executed. |
| Scope and reuse | The 84-path refactor is cohesive: shared CatalogMetaCache ownership and CatalogMetadataChanges replace scattered invalidation. No unrelated source change identified. |
| Concurrency and locks | Traced query loads, background refresh, HMS events, eviction, ALTER and DROP/rename. Publication uses per-entry bulk phases, the shared root phase and key nodes; nested root reads are reentrant. Removal callbacks drain after publication locks. No additional reachable lock cycle established. Remote loading occurs outside the short publication barrier. |
| Lifecycle | Database eviction retires instance registrations; accepted operations pin auxiliary indices until completion. Old cleanup uses exact identities. Terminal catalog retirement prevents resurrection; registry publication explicitly reactivates rename/replay. No C++ static initialization involved. |
| Configuration | No new configuration declaration. Existing TTL, refresh, count and weight policies are reused; property updates retire the appropriate runtime/group and subsequent access uses the updated policy. |
| Compatibility | Connector API major changes from 19 to 20 with matching recorded plugin surface. New journal identity/scope fields are optional Gson additions; old records use existing replay fallbacks and broader remote invalidation when identity is unavailable. |
| Thrift/protobuf | No changed definitions, field IDs or enum values. |
| Parallel paths | Checked direct refresh, external writes/DDL, structural and partition events, new/old journal replay, cold and mapped identities, sibling domains, default schema caching and Remote Doris. The rename-destination canonicalization gap is the one accepted new issue. |
| Conditional logic | Cache-only replay, disabled-cache transient access, content-versus-lookup scope and retirement guards have concrete lifecycle reasons. Destination canonicalization is missing from the rename branch. |
| Test coverage | Inspected latch-based late-publication/retirement tests, owner replacement and rename/replay tests, identity-index tests, row-count refresh/retention tests, connector/SPI tests, and Hive refresh/recreation regression coverage. No new production issue established from the remaining test changes. |
| Expected results | Both changed regression outputs agree with the suite operations and intended membership behavior. Auto-generation provenance and runtime results were not independently verified. |
| Observability | Root/domain entries are included in cache statistics; projection/load failures have existing logging and propagation. Missing table handles now preserve a table-not-found error. No additional observability finding established. |
| Persistence and failover | Checked create/drop database/table, truncate and rename journal writes and Env/EditLog dispatch. Persisted remote identities support cold follower replay without connector initialization; legacy records have fallback paths. The inline rename issue affects leader and replay. |
| Writes, atomicity and crashes | Remote commits remain followed by journaling and local cache reconciliation; shared generation invalidation fences late publication. No internal transaction or storage-write protocol changes. No additional crash/failover leak identified by static tracing. |
| FE/BE variables | No new transmitted variable or FE/BE protocol change. |
| Performance and memory | Projection is cache-only; shared resource generations avoid manual sibling lists. Exact retirement and the global row-count retention policy were checked; payload-byte budgets remain distinct from count-only handles. No benchmark or runtime memory measurement performed. |
| Core invariants and other issues | The accepted issue concerns stale external metadata. Internal visible versions, MoW delete bitmaps and BE memory tracking are unaffected. Known candidates and risks have explicit dispositions in the ledger; no unresolved review candidate remains. |
Review limits: static inspection only, as required by this invocation. No builds, unit/regression tests, multi-FE failover, external-service execution or performance measurements were run. Author-reported test results are not claimed as independently reproduced.
Review converged after 2 round(s).
| } | ||
| if (change.getNewTableName() != null) { | ||
| scopes.add(local(ScopePath.table(change.getDbName(), change.getTableName()))); | ||
| scopes.add(local(ScopePath.table(change.getDbName(), change.getNewTableName()))); |
There was a problem hiding this comment.
[P2] Invalidate the case-equivalent rename destination
With lower_case_table_names=2, warm a destination named Dest, drop it outside Doris, then rename another table to DEST in a case-preserving Iceberg catalog. This branch invalidates only the exact DEST key and its numeric ID; the cached Dest object, ID, schema and row count survive. By-ID readers (ExternalDatabase.getTableNullable(long), used by statistics and constraints) can therefore still resolve the dropped destination after the rename. Previously invalidateTableRename() resolved both names through resolveTableNameForInvalidation(). Add the canonical destination's LOCAL and ID scopes to this barrier on the leader and replay paths, and cover this mode-2 replacement case.
TPC-H: Total hot run time: 28409 ms |
TPC-DS: Total hot run time: 150301 ms |
ClickBench: Total hot run time: 24.23 s |
FE Regression Coverage ReportIncrement line coverage |
2 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28758 ms |
TPC-DS: Total hot run time: 150829 ms |
ClickBench: Total hot run time: 25.19 s |
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Problem Summary: With case-insensitive external table lookup, a rename destination can have a different spelling and deterministic ID from a previously cached table at the same case-equivalent name. Invalidating only the new spelling leaves the old destination object and derived caches reachable by ID. Normalize LOCAL table resource identities at the catalog root using an immutable case policy, keeping keys and remote scopes unchanged. On mode 2 lookup invalidation, fence numeric derived state for the database to cover IDs published after planning. Retire the root when table-name mode changes. Live changes and replay share this publication barrier without remote I/O.
### Release note
Fix stale external table metadata after a rename replaces a case-equivalent cached destination.
### Check List (For Author)
- Test: Unit Test
- Reproduced stale destination identities in leader, cold-name replay and refreshed-name replay tests before the final fix.
- All 437 tests passed using run-fe-ut.sh: cache framework 173, connector SPI 82, related FE 182.
- Full FE build and Checkstyle passed using build.sh --fe -j4 with JDK 17.
- Behavior changed: Yes. Case-insensitive lookup invalidation clears case-equivalent local identities and recalculates row counts in the affected database; sibling objects and schemas remain cached.
- Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed #69458 at 8e4f3a06ce033214d9da2e14958069469520bb1e against the supplied aggregate diff. One new P2 finding: Fluss retains historical lake-path mappings across catalog refreshes, causing unbounded retention and growing dependency scans.
All four existing threads are addressed at this head: catalog rename/replay reactivates the registered instance; retired databases skip cache-based name resolution; refresh tests reacquire replacement objects; case-equivalent table rename destinations and numeric state are fenced. None of the previously reported P0/P1 findings still applies, and none is reposted. No additional user focus was supplied; the entire PR was reviewed.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and supporting tests | The implementation unifies catalog ownership and LOCAL/REMOTE/ID invalidation. New unit and regression cases cover refresh, replacement, replay and disabled caches; the Fluss mapping lifetime remains incomplete. |
| Scope and clarity | The broad refactor stays focused on ownership and invalidation, including connector migrations and test updates. |
| Concurrency and lock order | Query workers, refresh executors, DDL, event daemon and replay were traced. Resource publication uses short reentrant read phases, invalidation detaches in a writer phase, and cleanup runs afterward. No additional concrete deadlock or publication defect was verified. |
| Lifecycle | Exact registrations isolate retired instances from replacements; accepted FE operations pin their adapters. The inline Fluss finding identifies auxiliary state outside that lifecycle. |
| Configuration | No new deployment setting. Existing cache configuration and case-policy changes recreate the affected domains/root. |
| Compatibility | SPI major version moves to 20 with its surface baseline. Optional Gson journal fields retain old-record fallbacks; no new storage format was introduced. |
| Thrift and protobuf | Neither is changed. |
| Parallel paths | SQL refresh, content mutation, DDL, structural/partition events, cold replay and embedded sibling paths were checked. |
| Conditional checks and errors | Disabled caches, case modes, cold identities and projection failures were examined. Known dependencies are invalidated before projection errors propagate; no further verified error-handling issue. |
| Test coverage | Added tests exercise important positive, negative and concurrency paths. Missing coverage for Fluss auxiliary-map retirement is part of the inline finding. |
| Modified expected results | The Hive and nested-namespace outputs match the changed SQL expectations by inspection. Generation provenance and runtime results were not verified. |
| Observability | Existing cache metrics, registration/engine attribution and operation logs remain available; no additional concrete observability defect found. |
| Persistence and failover | New DDL records retain local/remote identities; replay applies the same semantic changes with legacy fallbacks. Committed remote mutation records precede fallible cache bookkeeping. |
| Writes, atomicity and crashes | This changes post-mutation metadata invalidation, not transaction commit/visibility protocols. Replacement roots and exact-registration cleanup were inspected; no further verified crash/replay defect. |
| FE-to-BE variables | No new transmitted variable or sending path. |
| Performance and memory | The inline P2 describes unbounded Fluss history and linear synchronized dependency projection. Catalog/global payload budgets, count-only handles and row-count retention were also reviewed. |
| Other issues and core invariants | No additional verified finding after independent reviews and missed-case sweeps. No BE allocation, partition-visible-version or delete-bitmap code is changed. |
Validation was static only, as required: no builds, tests, cluster execution or source modifications. No unresolved candidate or uncovered changed-file scope remains; runtime behavior and test execution remain unverified.
Review converged after 2 round(s).
|
|
||
| @Override | ||
| public void invalidateAll() { | ||
| metaCache.invalidateCatalog(); |
There was a problem hiding this comment.
[P2] Retire Fluss lake-path history on catalog refresh
After querying lake-enabled Fluss tables, dropping them externally and issuing REFRESH CATALOG, every source/lake TablePath pair now remains in lakeTablePaths. Previously catalog refresh called FlussConnector.invalidateAll(), which cleared this map; the new engine path invalidates the shared root directly, and the map is only cleared by close(). A long-lived catalog with table churn therefore retains all historical names indefinitely, and each database dependency projection scans that history under the connector monitor. Give this auxiliary mapping a lifetime coordinated with root catalog invalidation, preserving dependencies until sibling scopes are fenced. Restoring a clear only in this method is insufficient because engine refresh bypasses it; cover repeated catalog refreshes in a regression test.
### What problem does this PR solve? Issue Number: None Related PR: apache#69458 Problem Summary: A remote lookup could report a missing external table while FE continued to list its cached name. Observe the exact retained table and matching name snapshot before handle lookup, then conditionally invalidate their dependencies under one catalog publication writer. Reject observations after replacement, names refresh, content invalidation or registration retirement so a late missing response cannot evict a recreated table. Reuse the common dependency planner without writing a DDL journal. Preserve Paimon and Elasticsearch lookup failures instead of treating them as table absence. ### Release note Clear stale external-table names and metadata after a lookup confirms that the table is missing, while protecting same-name replacements from delayed responses. ### Check List (For Author) - Test: Unit Test / Regression test - Cache framework: 176 tests passed; focused FE missing-table, lifecycle and owner tests: 56 passed. - ES/Paimon failure classification and related FE system-table, row-count and metadata-change tests passed. - Full FE build and Checkstyle passed. Real HMS owner-refresh regression was generated and compared successfully (1 suite, zero failed/fatal/skipped). - Behavior changed: Yes. Confirmed missing-table reads repair this FE's cached lookup state. - Does this need documentation: No. Internal cache ownership contract is documented in the connector README.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69458 at 77b60ac4e47159920ea7937b83daa7fc59097f39, covering all 91 changed files and their relevant callers. Two new P2 findings are inline: valid Iceberg views authorize missing-table repair, and natural parent-database eviction leaves a delayed-negative observation window. No new P0/P1 finding was verified.
Existing review context was independently revalidated. The catalog-rename P1 (#4346749470), disabled-cache handling (#4346749481), refresh tests (#4346749491), and case-equivalent rename destination (#4351655200) are addressed at this head. The Fluss lake-path history P2 (#4355558692) still applies and is not duplicated. No existing P0/P1 remains applicable. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: the shared root/planner coordinates LOCAL, REMOTE and ID dependencies; read-observed absence remains incomplete in the two inline cases. Added tests demonstrate intended scenarios in source; they were not executed here.
- Scope and abstraction: this is a substantial but cohesive cache-ownership refactor. Instance domains, typed scopes and a common semantic planner reuse the shared cache framework.
- Concurrency: query loaders, background refresh, retention/removal callbacks, events and replay were traced. Shared publication fencing covers explicit invalidation and exact-key replacement; the natural parent-eviction gap is inline.
- Lock cost and order: loaders perform remote work outside publication phases; read reentrancy and deferred callbacks avoid the examined gate/index cycles. No separate new deadlock was substantiated.
- Lifecycle: domains retire exact registrations, accepted adapter operations pin auxiliary indices, and catalog rename reactivates ownership. Parent replacement must additionally fence the old missing observation.
- Configuration: no new setting is introduced. Table-name-mode and catalog-budget changes retire the root; schema-entry reconfiguration borrows it. Existing TTL/count and payload-weight policies remain explicit.
- Compatibility: connector API major 20 and its changed surface baseline are updated. Optional Gson journal fields have legacy fallback paths; mixed-version deployment behavior was inspected statically, not exercised.
- Thrift/protobuf: no changes under gensrc; cross-branch field-ID comparison is inapplicable.
- Parallel paths: explicit refresh, content writes, successful DDL, structural/partition events, replay, sibling connectors, RemoteDoris, system tables and session bypass were checked. The view path needs the inline correction.
- Conditional logic: case-mode canonicalization, cold replay fallback, bypass and retirement conditions have concrete roles. No further speculative-condition defect was verified.
- Tests: positive, failure, replay, replacement and latch-driven race coverage was inspected. Retained Iceberg-view binding and natural eviction before callback retirement need dedicated coverage.
- Result files: changed Hive and nested-Iceberg outputs are consistent with the supplied suites on static inspection. Their generation and runtime correctness were not independently verified.
- Observability: existing cache metrics, per-registration engine stats and contextual failures cover the changed mechanisms; no additional observability finding was substantiated.
- Persistence/failover: changed DDL carriers and Env/EditLog dispatch preserve remote identities and lookup intent. Committed DDL is journaled before fallible invalidation; old-log replay remains cache-only.
- Writes/crashes/atomicity: this modifies post-mutation metadata invalidation rather than transaction commit/version rules. Scope detachment is coordinated; the delayed-negative parent race is separately reported. No runtime crash/failover test was performed.
- FE/BE propagation: no new transmitted variable or protocol path is introduced. BE visible-version, delete-bitmap and allocator invariants are unaffected.
- Performance and other concerns: row-count retention remains globally bounded with root-owned futures, and failed automatic refresh retains successful values. View cache churn is inline; Fluss historical-map growth is already reported. No further verified issue remains outside the declared findings.
Validation was static only: no builds, tests, service execution or repository source changes. All changed paths and assigned risk focuses were covered; runtime and multi-FE behavior remain unexecuted validation limits.
Review converged after 2 round(s).
| return metadata.getTableHandle(session, dbName, getRemoteName()); | ||
| Optional<ConnectorTableHandle> handle = metadata.getTableHandle(session, dbName, getRemoteName()); | ||
| if (!handle.isPresent()) { | ||
| observation.ifPresent(CatalogMetadataChanges.MissingTableObservation::missing); |
There was a problem hiding this comment.
[P2] Exclude existing views from missing-table repair
For a valid Iceberg view, tableExists() is false while viewExists() is true. Such views are still constructed as PluginDrivenMvccExternalTable, and BindRelation.getLogicalPlan() calls loadSnapshots() before its isView() branch. An ordinary SELECT therefore reaches this new missing() call through materializeLatest(), evicting the valid view's object, ID, schema and parent table-name snapshot. Repeated view queries now force remote namespace listings and adapter recreation instead of retaining that metadata for its configured TTL. Exclude confirmed views from shared missing-table repair, or establish that neither a table nor a view exists, and cover a retained Iceberg view through the MVCC bind path.
| } | ||
| // Name refresh can change the canonical spelling without replacing the old exact table key. | ||
| return tableNames.observe("", names).flatMap(nameObservation -> | ||
| tables.observe(table.getName(), table).map(nameObservation::and)); |
There was a problem hiding this comment.
[P2] Include the retained parent database in the observation
These tokens only observe this database instance's child entries. During natural database-cache eviction, ScopedMetaCache physically removes the parent before draining the deferred removal listener that calls releaseCachedEntries(). If an old missing-table lookup is pending, another query can publish a replacement database and recreated table in that interval. The old children still have current registrations and retired == false, so the delayed negative passes this check and invalidates the replacement's table, schema, IDs and names through their shared scopes. Explicit-refresh generation fencing does not cover this natural-eviction path. Include the exact parent database registration in the observation, and cover a delayed eviction callback with replacement publication before the old negative completes.
What problem does this PR solve?
Issue Number: None
Problem Summary:
External catalog metadata is cached in several dependent layers: database/table membership, FE object and ID lookup, schemas, row counts, and connector metadata such as partitions, files, and snapshots. Previously these layers had separate owners and callers manually invalidated both sides. A refresh could leave a stale membership or object lookup behind; a load started before invalidation could republish an obsolete object or identity, particularly after a drop and same-name recreation. Keeping the old FE database object after eviction also allowed its cleanup to affect a replacement.
Use one
CatalogMetaCacheroot per external catalog. FE registrations and instance-owned connector domains share its resource generations and existing budgets. Embedded connectors use child domains. Retiring an instance removes only its exact registrations; semantic resource invalidation is independent of instance lifetime. Local names, remote names, and numeric derived-state identities use distinct coordinate spaces.CatalogMetadataChangesis the common boundary for refresh, successful writes/DDL, metastore events, and replay. It projects affected local and remote resources and invalidates them at one publication barrier. Connectors describe cross-resource dependencies through a pure SPI projection instead of imperative sibling invalidation. DDL journal records preserve remote identities for replay; old records fall back to broader remote-only invalidation without initializing remote metadata. Content changes preserve table-name snapshots and FE lookups, while explicit table/database refresh also invalidates the appropriate membership snapshot.ID mappings survive ordinary object eviction but participate in semantic invalidation. Publication is guarded by the same resource generation, and accepted operations pin their auxiliary index until completion. Row-count futures belong to catalog roots while a shared retention-policy index preserves the existing process-wide entry limit. Cache callbacks retain the owning plugin classloader context.
Automatic row-count refresh keeps the last successful value while reloading and on failure. ADBC membership caches register under the shared collection scopes.
For case-insensitive table lookup (mode 2), the root captures an immutable normalization policy for LOCAL table resource scopes. Cache keys and displayed names retain their original spelling, while case-equivalent local identities share the same invalidation fence. REMOTE scopes keep their case semantics. Lookup invalidation (including rename and its event records) additionally fences numeric derived state for the affected database, covering a first object/row-count load that publishes an old spelling after invalidation planning. Sibling FE objects and schemas remain cached; row counts in that database are recalculated. Changing the table-name mode retires the old root and its policy. Planning performs no remote metadata lookup.
This also addresses catalog rename/replay reactivation, disabled database caching with case-insensitive table lookup, and tests that continued to use retired database instances. The connector plugin API major version changes from 19 to 20 with updated surface coverage.
When an ordinary base-table handle lookup definitively reports absence, FE now repairs its stale membership, object, ID and connector dependencies through the same planner. A short-lived observation binds the exact retained table and its matching names snapshot before the remote call. Scope/registration checks and dependency detachment run in one publication write phase; logical retirement, name refresh, content invalidation or replacement reject a delayed negative result. This also protects differently cased replacements such as
FootoFOO. Read-observed absence only repairs the current FE and does not write a DDL journal. The first failed MVCC query can still report the generic schema-cache load error after the missing handle has retired its object; this change repairs cache state and does not unify missing-table error messages. Paimon and Elasticsearch propagate ordinary lookup failures instead of reporting them as table absence.Release note
Fix stale external-catalog table membership and metadata after refresh or a confirmed missing-table lookup, including drop and same-name recreation. Ordinary data writes keep cached table-name listings.
Check List (For Author)
./build.sh --fe -j4with JDK 17.test_hive_catalog_owner_refreshwas regenerated against the newly packaged isolated FE and real HMS, then passed without generation: 1 suite, zero failed/fatal/skipped. It checks explicit refresh and automatic membership cleanup after an observed missing-table lookup.be_exec_version=13; it does not replace current-head CI validation.fe/fe-connector/README.md.