Repository navigation
Conversation
### What problem does this PR solve?
Issue Number: None
Problem Summary: Explicit REFRESH TABLE invalidated the connector metadata but kept the external database's cached table-name snapshot, table object, and ID lookup. After a remote table was dropped or recreated, SHOW TABLES and later lookups could keep using the old identity until cache expiry. Invalidate that lookup state for explicit refresh on the leader and during follower replay, including cold replay paths. In case-insensitive mode, invalidate the database scope so an old spelling cannot retain a stale object. Keep content-only refreshes unchanged and treat old edit logs without the new flag as content-only refreshes.
### Release note
Explicit REFRESH TABLE now reloads external table names and table lookup identity.
### Check List (For Author)
- Test: Unit Test
- ExternalDatabaseTest and RefreshManagerTest: 52 tests passed.
- FE build and Checkstyle passed.
- Behavior changed: Yes. Explicit REFRESH TABLE reloads external table lookup state.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
…fresh
### What problem does this PR solve?
Issue Number: None
Problem Summary: Explicit REFRESH TABLE with case-insensitive table lookup invalidates all FE table identities in the database, but a hot-table replay could invalidate only the selected connector table spelling. If another spelling already had a cached connector snapshot, queries could continue reading that snapshot after refresh. Invalidate the connector database before clearing FE lookup and metadata caches in this mode, while preserving single-table invalidation for other refreshes.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- RefreshManagerTest: 16 tests passed.
- FE build and Checkstyle passed.
- Behavior changed: Yes. Explicit case-insensitive REFRESH TABLE invalidates database-scoped connector caches.
- Does this need documentation: No
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
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.
Static review of all five changed files at head 0da4c6d found one P2 concurrency defect: mode-2 explicit refresh can erase a newly published table ID while its new object remains cached. No P0/P1 finding or existing blocking thread was found. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: explicit REFRESH TABLE now clears names, table objects and lookup IDs; internal content refresh retains identity. The new unit tests demonstrate sequential clearing, case-equivalent replacement and replay behavior, but do not cover the reported publication race.
- Scope and reuse: three production files and two test files form a focused change using existing invalidators and the existing refresh log operation.
- Concurrency and locking: SQL lookup, replay, event and cache-refresh workers can overlap. Individual cache stripes, scope-generation barriers and the ID-index lock protect their own state; bulk object invalidation and later ID clearing lack a shared publication window. No additional deadlock or heavy IO under a new lock was identified.
- Lifecycle: replay remains cache-only and lazy; explicit refresh discards cached identities while existing query references can remain live. No new resource cycle, executor or static initialization dependency was introduced.
- Configuration: no setting was added; existing lower_case_table_names selects database-scoped invalidation in mode 2.
- Compatibility: the annotated primitive Gson flag defaults to false for old records; the operation and serialization envelope are unchanged. Legacy records retain content-only refresh behavior.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: explicit SQL and hot/cold replay propagate the new flag; mutation, INSERT, DDL, truncate and event refreshes retain the false-default overload. Connector fallback invalidation avoids synchronous remote loading on cold replay.
- Conditions and errors: the cold-initialization guard and mode-2 broadening match existing lookup semantics. Nested finally blocks retain FE and engine/row-count barriers when connector invalidation fails, and failures propagate.
- Tests and negative cases: JUnit 5 additions cover explicit/content separation, cold objects, database-scoped mode-2 invalidation, flag round-trip and case-equivalent identities. A deterministic test for the reported object/ID publication gap is missing; no end-to-end SQL/schema validation was added.
- Expected results and validation: no regression output files changed. Per the review contract, tests and builds were not run; validation here is static inspection only.
- Observability: existing logs identify catalog, database, table/ID, update time and replay warmth; no additional metric requirement was identified.
- Persistence and failover: the explicit flag is written and read through ExternalObjectLog and the existing EditLog dispatch. Master/replay invalidation scopes match; committed external mutation logging remains before fallible refresh. No additional durable metadata mutation was introduced.
- Data writes and crash behavior: no remote data write, transaction, visible-version or delete-bitmap logic changed. The local cache atomicity defect is the inline finding; no separate crash-handling defect was found.
- FE/BE propagation: no new cross-process variables or wire fields.
- Performance: mode-2 database-wide invalidation deliberately broadens work to fence case-equivalent identities; no separate substantiated performance defect was found.
- Other invariants and concerns: no BE memory-accounting or function-system change applies. Mapped names, session bypass and retained-ID consumers were checked. No additional naming/replay defect or unresolved concrete candidate remains.
Coverage limit: static review cannot establish runtime/build success; the reported interleaving is supported by the reachable publication and invalidation control flow. All changed paths and identified risk focuses were reviewed.
Review converged after 3 round(s).
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: A catalog-wide table cache invalidation could clear the table ID index after a concurrent lookup published a fresh table. The table object would remain cached while its ID lookup disappeared. Run the index clear at the same publication boundary as cache invalidation, and route warm-value index repairs through that boundary. A concurrent lookup test covers the clear and republish ordering. ### Release note None ### Check List (For Author) - Test: Unit Test: FeMetaCacheEntryTest, ExternalDatabaseTest, RefreshManagerTest (72 passed); FE build and Checkstyle passed. - Behavior changed: Yes (table ID lookup remains consistent across concurrent bulk invalidation) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all nine changed files and the related refresh, replay, name-resolution, cache-publication and row-count paths at 8bd1d68d65ac063430ba54c21e14c32e4288795c. No new PR-introduced finding was accepted; no existing P0/P1 finding applies.
The specific post-invalidation/pre-ID-clear interleaving in existing P2 thread 4356099602 is closed: bulk ID clear shares the table-cache publication gate, and warm index repair uses guarded CAS. A separate delayed-action-token/new-lease window remains in unchanged adapter code: a lookup paused after creating its FE token can acquire a fresh cache lease after invalidation, publish via commit(NO_OP), and skip its ID repair. Comparison with the merge base confirms that this behavior already existed, including the database-reset trigger. It is retained as known context for the same object/ID invariant and is not reposted as a new inline finding. Disabled-cache callback and sibling catalog-index limitations also predate this diff.
The reviewed control flow is:
explicit REFRESH TABLE
-> connector invalidation
-> FE names/object/ID invalidation
-> engine metadata invalidation -> row-count invalidation
-> journal explicit-refresh flag -> follower warm/cold replay
mutation/event refresh -> existing content-only invalidation
| Critical checkpoint | Conclusion |
|---|---|
| Goal and supporting tests | Explicit refresh now resets membership and table lookup identity; content refresh retains its owner. Added unit tests cover eviction, replay and case-equivalent names. Source supports the intended change; tests were inspected only. |
| Scope and clarity | Focused refresh intent flag and reuse of shared invalidation/publication mechanisms; no unrelated change found. |
| Concurrency and protection | Query workers, refresh/replay threads, metastore events and async name refresh can overlap. Entry stripes/action tokens, scope generations, publication gates and the ID-index mutex protect the reviewed transitions, subject to the preexisting token/lease limitation above. |
| Work inside locks | Remote loading and connector calls remain outside the new gate callback. ID-map clear runs within the writer; detached-value cleanup runs afterward. |
| Lock order and deadlocks | Publication takes entry stripe before cache guards and index mutex. Bulk stripe scans release each stripe before the writer; the actual clear callback never reenters caches. No new lock cycle identified. |
| Lifecycle and initialization | Ownership stays with existing cache entries; detached registrations are cleaned by identity. Replay misses avoid synchronous remote initialization. No new static-initialization dependency or resource owner. |
| Configuration | No new configuration; existing case policy intentionally selects table versus database invalidation. |
| Compatibility | The additive Gson flag defaults false in older logs; existing three-argument internal refresh callers retain content-only behavior. No connector SPI surface change. |
| Thrift/protobuf | No definitions changed. |
| Parallel paths | Warm/cold database and table replay, modes 0/1/2, mappings, mutation/event/DDL/truncate paths, by-ID access, eviction and reset were checked. No additional changed-flow defect verified. |
| Conditional branches | The mode-2 widening handles distinct preserved-case owners; initialization guards avoid unbuilt caches. Existing validation/error conventions are preserved. |
| Test coverage | JUnit tests cover explicit versus content refresh, cold replay, alternate spelling and bulk publication. The new lookup-start latch precedes the entry call, so it does not force every publication overlap; it is not proof of the broader object/ID invariant. No live connector end-to-end validation. |
| Test results | No golden result files changed. Added assertions were checked against source; no test execution or build result is claimed by this review. |
| Observability | Existing refresh/replay logs and cache metrics cover the changed paths; no additional instrumentation defect found. |
| Persistence and failover | The existing journal operation writes/reads the new intent flag, and replay follows matching invalidation scope. Mutation logging before fallible refresh is preserved; cold replay invalidates conservatively. |
| Data writes and crash behavior | No new remote data-write or transaction path. Nested finally blocks preserve local and row-count barriers on invalidation failure; runtime caches rebuild after restart. |
| FE/BE propagation and core storage invariants | No FE/BE field, committed-data visibility, partition-version, delete-bitmap or BE memory-ownership change. |
| Performance | Ordinary indexed warm lookup still returns without CAS. Database-wide invalidation is limited to explicit mode-2 refresh; index clearing is proportional to index size. No new avoidable hot-path IO or scan found. |
| Other concerns and user focus | No additional user focus was supplied. Follow-up checks covered delayed load initiation, stale names refresh, mapped spellings, update time, callback failures and cleanup ownership. No unresolved new candidate remains. |
Validation limits: static review only, with no builds, tests or source modifications. The known preexisting races and the concurrency-test limitation above are classified limits, not unverified new findings or unreviewed changed scope.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Explicit external-table refresh now invalidates the cached table object. The existing test continued to read the old object after refresh and expected its schema load to advance the old update time, causing FE UT to fail. Resolve the table again and verify the replacement is reachable by both name and ID while the old object's update time stays unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test: RefreshTableTest, FeMetaCacheEntryTest, ExternalDatabaseTest, RefreshManagerTest (74 passed); FE build and Checkstyle passed. - Behavior changed: No (test expectation only) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of head de5afec found three new P2 issues: numeric-lookup ID publication, replacement update timestamps, and mode-1 historical remote-key invalidation. No P0/P1 finding was identified.
- Goal and scope: explicit REFRESH now invalidates FE name membership and object identity, while content/event refresh remains narrower. All 10 changed files and related callers were reviewed; the changes are focused and reuse the existing cache/refresh framework. The inline findings describe the remaining behavior gaps.
- Concurrency and locks: SQL/query threads, row-count loaders, replay, name refresh, and incremental events were traced. Named cold/warm publication and bulk ID clear share the gate; the numeric path remains uncovered. IO and detached cleanup stay outside the publication gate, with no new lock inversion found.
- Lifecycle: lazy cache creation, old references, generation fencing, eviction, and detached ownership cleanup were checked. Replacement objects lose the refresh timestamp. No new static-initialization or resource-cycle issue was found.
- Configuration and conditions: no new configuration was added. Existing modes 0/1/2, mapped names, session cache bypass, and disabled caches were checked. The mode-2 broad-invalidation condition omits mode-1 historical remote spellings.
- Compatibility and persistence: the additive Gson flag defaults false for old logs; explicit refresh writes it and both hot/cold replay paths consume it without synchronous remote loading. Mutation, truncate, DDL and event refresh retain their content semantics. No gensrc field, storage format, FE/BE variable, or data-transaction write changes are involved; visible-version and delete-bitmap invariants are unaffected.
- Errors and observability: nested finally blocks preserve FE and row-count invalidation after connector failure. Existing identity-bearing logs remain adequate; no new metric requirement was identified. The visible UPDATE_TIME regression is inline.
- Tests and results: the new JUnit coverage checks name/object replacement, ID lookup, cold replay, mode-2 connector invalidation, JSON round trip and bulk named publication. It misses numeric capture/resume, timestamps before schema load, and mode-1 historical connector keys. The new lookup-start latch precedes the actual cache call, so it does not force gate contention on every schedule. No regression result files changed. Per the review contract, no build, tests or runtime experiments were executed.
- Performance, reuse and remaining checks: hot named hits keep their fast path; warm repair adds bounded publication work; bulk ID clearing is proportional to retained identities and cleanup remains outside the gate. No additional substantiated performance, memory-ownership, or error-handling defect was found.
Existing thread 4356099602 was independently revalidated: its original mode-2 named-publication/late-clear interleaving is fixed. It is not reposted; the numeric-path finding is a separate single-key case. There are no existing P0/P1 comments to retain. No additional user focus was supplied. Coverage is complete for a static review; execution remains unvalidated as required by the environment.
Review converged after 3 round(s).
| } | ||
| String localTableName = resolveTableNameForInvalidation(tableName); | ||
| try { | ||
| invalidateTableCache(localTableName); |
There was a problem hiding this comment.
[P2] Preserve ID publication for in-flight numeric lookups
With enabled caches and ordinary lower_case_table_names=0, an ExternalRowCountCache worker can enter getTableNullable(long), read the indexed name at line 779, and pause before tables.get(name). This new per-table invalidation can then finish removing the object and its ID. When the worker resumes, tables.get acquires a fresh post-refresh generation and caches a replacement object, but that plain getter never restores the ID mapping. Subsequent numeric lookups fail despite the object being cached, and row-count/statistics callers return unknown until a lookup by name repairs it. Make numeric loads revalidate or publish their ID within the guarded object-publication action, and cover this capture/refresh/resume interleaving. This is distinct from the existing mode-2 late bulk-clear thread: it uses the single-key branch and publishes through the numeric path after ID removal has completed.
| Env.getCurrentEnv().getExtMetaCacheMgr().invalidateTable(table); | ||
| try { | ||
| if (invalidateTableLookup) { | ||
| db.invalidateTableLookup(table.getName()); |
There was a problem hiding this comment.
[P2] Retain the refresh timestamp for the replacement object
refreshTableInternal writes updateTime onto the old table immediately above, but this call evicts that object. Its replacement starts with ExternalTable.updateTime = 0, so REFRESH TABLE followed by an information_schema.tables query projecting only UPDATE_TIME now returns NULL instead of the refresh time: FrontendServiceImpl reads the field directly, and the schema scanner converts zero to NULL. It stays unset until a later schema load calls initSchemaAndUpdateTime. Preserve the refresh time across replacement and test it before loading schema; the modified test currently checks the new timestamp only after getFullSchema().
|
|
||
| private void refreshTableInternal(ExternalDatabase db, ExternalTable table, long updateTime, | ||
| boolean invalidateTableLookup) { | ||
| boolean invalidateDatabase = invalidateTableLookup && table.getCatalog().getLowerCaseTableNames() == 2; |
There was a problem hiding this comment.
[P2] Invalidate historical remote spellings in mode 1 as well
A local name can resolve to a different remote spelling with lower_case_table_names=1 too. For example, configure FE object/name expiry shorter than the Iceberg connector table TTL, query remote Foo as local foo, externally replace it with FOO, and query foo after FE-only expiry. The FE now retains FOO while the independent connector entry for Foo survives. Externally replace FOO with a new Foo containing different data, then refresh before the FE FOO object expires: this condition is false, so only connector key FOO is invalidated. The refreshed names then resolve back to Foo, whose exact-case table/snapshot caches still describe the first incarnation, allowing stale schema/data after successful refresh. Extend the broad invalidation to naming modes that can change the remote spelling, and cover this historical-key reuse case.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Problem Summary: A live connector lookup can confirm a remotely dropped table while FE names, objects, IDs and metadata still retain its old identity. Capture exact retained cache observations before the remote read, and conditionally invalidate matching state only while the catalog, database, names and object registrations are still current. This prevents an old missing response from clearing a refreshed or recreated table. Defer physical removal and resource release until all enclosing guards are released. Keep ordinary write refreshes narrow. Also preserve numeric ID publication across refresh, cover both case-folding modes on explicit refresh/replay, and initialize the timestamp on successful object reload. ### Release note External table lookups automatically clear matching stale metadata after a live lookup confirms a remote drop. ### Check List (For Author) - Test: Unit Test: 313 cache/FE cases and 33 ES/Paimon cases; Hive metadata regression with generated output and comparison; ./build.sh --fe -j4 including Checkstyle - Behavior changed: Yes, definitive missing-table observations invalidate matching external lookup state; ordinary writes do not reload table names - Does this need documentation: No
924060929
changed the title
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 25 changed paths at a099bd2 found one new P2 issue: valid Iceberg views reach the table-handle absence branch and unnecessarily lose their shared lookup and metadata caches. The finding is inline. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: explicit refresh now invalidates names/object/ID lookup state, and definitive missing base-table reads conditionally repair it. Added tests cover these intended behaviors; the healthy-view exception remains.
- Scope and reuse: changes stay within external metadata refresh and reuse the existing scoped-cache publication and removal mechanisms.
- Concurrency: traced query/preload, MVCC, row-count, refresh/replay and cache-worker entry points. Catalog -> database observation -> names -> object guard order matches incremental publication; no additional deadlock or replacement-generation defect was substantiated. Remote reads occur before the guarded cleanup; physical resource release occurs afterward.
- Lifecycle: exact retained registrations reject replaced/recreated objects; detached cleanup preserves new publications, drains after failures and clears its thread-local state. The actual connector classloader policy shares the cache library. C++ static initialization is unaffected.
- Configuration: no new settings. Existing case modes, name-miss policy and session cache bypass paths were inspected.
- Compatibility: the additive Gson refresh flag defaults to false for old records; ordinary content refresh retains its prior behavior. The SPI change is documentation only, with no method-surface change or incompatible storage-format change.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: warm/cold replay, ordinary writes/events, name/numeric lookups, system tables and connector failure handling were inspected. SDK views require the additional distinction described inline.
- Conditional checks: identity, generation, initialization and bypass checks correspond to concrete replacement/reset or session behavior; no additional incorrect condition was verified.
- Test coverage: new tests cover explicit refresh, stale missing responses, table/database/catalog replacement, ID publication, naming modes, cleanup failure, bypass, sibling retention and ES/Paimon error propagation. Concurrency tests use latches/futures. The added fixture does not exercise healthy SDK views through production MVCC binding.
- Changed results: the Hive expected-output sections match the suite's listing, schema and drop/recreate steps on static inspection. Output generation and runtime correctness were not independently reproduced.
- Observability: existing cache/load diagnostics and refresh logs remain; ES failures now include the index and HTTP status, and Paimon failures include the table. No additional required instrumentation was identified.
- Persistence/failover: explicit refresh journals its lookup-invalidation decision and replays both warm and cold states; older and mutation records retain false semantics. Automatic missing cleanup changes ephemeral local caches.
- Data writes/atomicity: no new durable-data mutation or transaction protocol is introduced. Auxiliary ID clear/repair shares the publication boundary, and ordinary external writes retain narrow invalidation.
- FE/BE passing: no new variables or wire payloads cross the boundary.
- Performance: the reported view case forces unnecessary database-wide names enumeration and metadata reloads. Other observation setup is constant-time, and detached cleanup retains its existing traversal outside the guards.
- Other concerns and limits: the complete changed-file list and upstream/downstream paths were covered statically; no builds, tests or source modifications were performed in this review environment.
Existing-thread disposition: the reported mode-1 historical-spelling case (4356914869), replacement timestamp (4356914864), numeric ID publication (4356914860), and bulk ID-clear ordering (4356099602) are addressed at this head and were not reposted. No existing P0/P1 finding was confirmed.
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(Runnable::run); |
There was a problem hiding this comment.
[P2] Preserve healthy Iceberg views when a table handle is absent
A normal SELECT * FROM iceberg_catalog.db.v on an existing SDK view reaches this branch: Iceberg advertises both MVCC and view support, so its views use PluginDrivenMvccExternalTable, and BindRelation.getLogicalPlan calls StatementContext.loadSnapshots before checking isView(). materializeLatest() then calls this resolver. Iceberg's tableExists() is false for views, so an empty table handle is expected here, as the separate view path in initSchema() already documents. With warm names/object caches, the new observation nevertheless removes the healthy view's object, ID and schema/connector metadata and invalidates the entire database names snapshot. Repeated view queries therefore force full table/view enumeration and rebuild cached metadata even though nothing changed remotely. Exclude known views from missing-base-table cleanup (or skip their table-only MVCC probe), and cover repeated view binding with a cache-retention test.
There was a problem hiding this comment.
Fixed in 584871c: an empty table handle now triggers missing-table cleanup only when live viewExists is also false. The new test checks repeated healthy-view lookups retain name/object/ID/connector caches, then removes the view and verifies cleanup still occurs.
### What problem does this PR solve? Issue Number: None Problem Summary: Iceberg views have no table handle, but the new missing-table observation treated that normal result as a remote drop and evicted healthy view name, object, ID, and connector metadata. Confirm live view existence before clearing a missing table lookup, while still cleaning a truly dropped view. Update the Paimon authentication test for its explicit error propagation, and cover numeric lookup publication, refresh timestamps, and historical remote spellings with focused tests. ### Release note Avoid unnecessary cache invalidation when querying existing external views. ### Check List (For Author) - Test: Unit Test: 77 targeted Paimon and FE core tests passed via run-fe-ut.sh; Checkstyle passed. - Behavior changed: Yes (healthy views retain external lookup caches when a table handle is absent). - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 26 changed paths at 584871c. No new actionable findings were verified. The five existing P2 concerns are addressed on this head: healthy views retain lookup caches, both case-folding modes clear historical connector spellings, replacement objects receive an update timestamp before schema loading, numeric loads republish IDs, and bulk ID clearing shares the object-publication barrier. No existing P0/P1 finding remains to carry forward; no duplicate inline comments are added.
Critical checkpoint conclusions:
- Goal and evidence: explicit REFRESH TABLE invalidates names/object/ID lookup state and journals that decision; confirmed missing base-table handles conditionally repair matching retained state. Unit and Hive regression additions cover refresh, drop/recreate, sibling retention and missing observations; inspected statically.
- Scope and reuse: changes stay within external metadata refresh, absence/error handling and shared cache publication/cleanup abstractions. Existing cache generation and CAS mechanisms are reused.
- Concurrency: query/statistics workers, refresh/event/replay threads and cache loaders were traced. Parent database -> names -> object guards and stripe/publication barriers reject stale observations and order ID updates. Remote handle/view reads precede guarded cleanup; detached removal and resource callbacks drain after enclosing guards. No new lock inversion was verified.
- Lifecycle: exact registration identity protects replacements, including reinsertion and delayed cleanup. Close/currentness checks reject retired observations; deferred cleanup drains on failure and clears its ThreadLocal. Connector cache classes share the parent loader. C++ static initialization does not apply.
- Configuration: no configuration item is added; existing cache, naming and session-bypass settings govern the new paths.
- Compatibility: the annotated additive JSON boolean defaults false for legacy logs. Existing refresh records retain content-refresh behavior; the new factory records explicit lookup invalidation. SPI changes are documentation only, with no method-surface change.
- Thrift/protobuf: no definitions changed.
- Parallel paths: checked warm/cold database and table replay, modes 0/1/2, named/numeric lookup, ordinary writes/events, schema/MVCC probes, views and the separate system-table resolver. No additional defect was verified.
- Conditions: retained names/object/database and publication checks have concrete refresh/replacement races to guard. Live view existence prevents an expected missing table handle from evicting a healthy view. ES and Paimon lookup failures now propagate; the other connector absence probes were traced.
- Test coverage: added latch/barrier tests exercise delayed missing responses, replacement database/catalog/table, exact-key replacement, bulk/single load fencing, ID publication and cleanup failure. Negative cases cover lookup errors, views, case modes and bypass sessions.
- Expected results: the Hive output names, schemas, empty listing and expected-error flow agree with the Groovy sequence. Output generation and author-reported test/build results were not reproduced by this static review.
- Observability: existing refresh/error logs identify affected objects; cache counters remain associated with synchronous cleanup. No additional observability requirement was substantiated.
- Persistence and failover: explicit refresh writes ExternalObjectLog and OP_REFRESH_EXTERNAL_TABLE dispatches to matching replay branches without synchronous replay-time remote loads. Automatic missing-table repair changes local ephemeral cache state. No transaction persistence change was introduced.
- Writes and crash behavior: ordinary committed mutations keep narrow content refresh and their existing log-before-fallible-invalidation path. Cache object/ID publication is guarded; no new persisted-data atomicity issue was verified.
- FE/BE variables: none added; no serialization/send-site change is required.
- Performance: healthy lookups add bounded local observation work; broader database invalidation in case-folding modes intentionally clears ambiguous historical spellings. Expensive physical cleanup is deferred beyond observation guards. No performance defect was established; no benchmark was run.
- Core invariants and other issues: no new data-visibility/error-handling violation was verified in the reviewed paths. BE visible versions, delete bitmaps and native memory tracking are unchanged. The final recheck found no additional actionable issue.
User focus: none was supplied beyond whole-PR review. Intended boundary: automatic missing-table cleanup requires a retained matching names snapshot and does not evict shared caches for schema-bypass sessions. Validation limit: source and test inspection only; no builds, tests, live-cluster execution or runtime performance validation were performed.
Review converged after 2 round(s).
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Problem Summary: Connector table tests used real table methods on mocks without an ExternalDatabase. The table lookup observer now requires that database, so 15 FE unit tests failed with a null pointer. Give the affected test fixtures a database mock that returns no cache observation, matching the supported lookup path while keeping the test focus on connector behavior. ### Release note None ### Check List (For Author) - Test: Unit Test (PluginDrivenExternalTableTest, 47 tests passed) - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69501 at 738c1a7: two new P2 findings. A delayed missing-table response is not fenced by an ordinary metadata refresh, and the Paimon assertion rewrite leaves two imports that fail the mandatory Checkstyle gate. No new or existing P0/P1 finding was confirmed.
Critical checkpoint conclusions:
- Goal and proof: explicit refresh now replaces lookup identity and missing observations repair retained lookup state. The ordinary-refresh race in the inline finding still violates stale-response protection; its interleaving lacks a regression test.
- Scope and reuse: changes are focused on lookup invalidation and absence/error semantics, using shared observation and invalidation helpers. No unnecessary connector-specific engine branch found.
- Concurrency and locks: query/schema/MVCC and background loaders race with refresh/replay/events. Catalog -> database entry -> names -> object ordering and the publication/index barriers were traced. Remote reads precede guarded mutation and resource callbacks are deferred until hierarchy locks exit. External tables inherit no-op TableIf locks, allowing the reported query/ALTER race. No additional lock-order defect found.
- Lifecycle: exact retained registrations, initialized/catalog identity checks and detached ownership fence replaced objects/catalogs/databases and protect new values during old cleanup. Cleanup drains after failures; timestamp initialization covers replacement objects. Metadata-only refresh needs the extra fence described inline; no static ownership cycle found.
- Configuration: none added. Existing naming modes 0/1/2 and shared-cache bypass paths reviewed.
- Compatibility: ExternalObjectLog adds a Gson boolean defaulting false for old records; the existing log opcode remains. SPI change is documentation only; no changed method symbols or durable table storage format.
- Thrift/protobuf: unchanged; branch field-ID checks are inapplicable.
- Parallel paths: reviewed explicit versus mutation/event refresh, warm/cold database/table replay, numeric/named lookup, naming modes, views, synthetic tables, session bypass, schema/MVCC/statistics and write-handle callers.
- Conditional/error paths: absence requires empty table handle and false live view existence; ES/Paimon failures now propagate. Retained membership/object/catalog checks are concrete, but do not detect the reported content refresh.
- Test coverage: inspected new deterministic race, replacement, failure, view, naming, replay, timestamp and sibling-retention tests plus the Hive regression. The ordinary-refresh stale-response interleaving is missing; the Paimon unused imports prevent normal validation.
- Expected results: added Hive schema/name output matches the scripted transitions, with ordered table listings. Output-generation provenance and runtime behavior were not independently verified.
- Observability: existing identifier-bearing refresh/error logs and cache counters remain applicable. No additional logging/metric defect found.
- Persistence/failover: explicit lookup invalidation is persisted through OP_REFRESH_EXTERNAL_TABLE and replayed in warm/cold branches; mutation logging remains before fallible refresh. No synchronous remote load added to replay; no transaction protocol change.
- Data writes and crashes: no new remote write/transaction logic. Cache reconstruction remains source-driven; no new committed-data loss or crash leak was identified.
- FE/BE propagation: no new variables or wire send paths.
- Performance: captures are cache-only; view existence is probed only for an empty handle. Full database invalidation is confined to explicit folding modes; the stale response inline finding causes unnecessary enumeration and cache rebuilding after content refresh.
- Further recheck: revisited all changed paths, actual query/ALTER concurrency, generation guards, cleanup exceptions, imports/build configuration and duplicate relationships. No additional verified finding.
Existing threads independently rechecked: bulk ID cleanup (4356099602), numeric ID publication (4356914860), replacement UPDATE_TIME (4356914864), mode-1 historical remote spellings (4356914869), and healthy Iceberg views (4357858293) have their concrete fixes present at this head. They are not reposted. All supplied originals are P2, so existing_blocking_comment_ids is empty.
User focus: no additional focus was supplied. Coverage limits: static inspection only, as required; no builds, tests, BE execution or external-service validation was performed. No unresolved candidate or uncovered changed-file scope remains after the final reviewer pass.
Review converged after 2 round(s).
| != extCatalog) { | ||
| return; | ||
| } | ||
| database.runIfCurrent(() -> membership.invalidateIfCurrent(() -> |
There was a problem hiding this comment.
[P2] Fence missing observations across metadata-only refreshes
These registrations stay current across refreshTableInternal(..., false). With warm Iceberg names/object caches, let query A receive a delayed missing-table response during an external drop, recreate the same table, then successfully run ALTER TABLE t ADD COLUMN through Doris and reload its schema. afterExternalDdl refreshes connector/schema metadata while retaining the same FE database, names and object registrations. When A resumes, all three guards still pass (viewExists is false for a base table), so it evicts the freshly refreshed metadata, current table object/ID and entire names snapshot. Numeric lookups then fail until name lookup repairs the ID, and listings must enumerate the database again. External table locks are no-ops, so the query/ALTER overlap is possible. Advance/check a metadata-refresh generation while preserving ordinary-write name/object retention, and cover a delayed miss spanning that refresh.
There was a problem hiding this comment.
Fixed in bee0566. Metadata-only refresh now advances a transient generation on the retained table object. Missing-table cleanup compares the captured generation while holding the same catalog monitor used to advance it, so a delayed missing response cannot evict metadata refreshed after its lookup began. Added a delayed-miss/metadata-refresh interleaving test that verifies the schema, name snapshot, object, ID, and connector cache remain intact. The scoped FE tests pass.
| ctx.failAuth = true; | ||
| Optional<ConnectorTableHandle> result = metadata(ops, ctx).getTableHandle(null, "db1", "t1"); | ||
| Assertions.assertFalse(result.isPresent()); | ||
| Assertions.assertThrows(DorisConnectorException.class, |
There was a problem hiding this comment.
[P2] Remove the imports made obsolete by this assertion
Replacing the Optional<ConnectorTableHandle> local removes the last uses of both ConnectorTableHandle and java.util.Optional, but their imports at lines 21 and 34 remain. The inherited Maven Checkstyle execution runs during validate, includes test sources and enables UnusedImports with failure on errors, so normal Paimon/FE validation now fails before tests run. Remove both imports alongside this change.
There was a problem hiding this comment.
Fixed in bee0566. Removed the unused ConnectorTableHandle and Optional imports. The Paimon read-auth test class passes (12 tests), and the Maven Checkstyle validation completed successfully.
### What problem does this PR solve? Issue Number: None Problem Summary: A missing-table response started before an ordinary metadata refresh could still evict the refreshed schema, table object, ID, and name snapshot because the refresh intentionally retained the same object registration. Advance a transient per-table metadata generation before refresh invalidation and compare it during guarded missing-table cleanup under the same catalog monitor. A delayed miss now leaves the current refreshed metadata intact. Also remove two unused Paimon test imports that failed Checkstyle. ### Release note None ### Check List (For Author) - Test: Unit Test (97 tests across PluginDrivenMissingTableTest, PluginDrivenExternalTableTest, RefreshManagerTest, and PaimonConnectorMetadataReadAuthTest; Maven Checkstyle) - Behavior changed: Yes (stale missing-table responses no longer evict metadata refreshed after their lookup began) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 28 changed paths at bee0566. Two verified P2 correctness findings remain in automatic missing-table repair: historical case-equivalent state can survive repair, and an in-flight lookup can republish an absent table object/ID. Both are inline. No new or independently confirmed existing P0/P1 finding was identified.
The seven existing P2 threads were rechecked: obsolete Paimon imports, metadata-only refresh generation, healthy views, explicit-refresh historical spellings, replacement timestamps, numeric ID publication and bulk ID cleanup are fixed for their reported scenarios. The new automatic-path findings are distinct; those existing comments are not duplicated. There was no additional user-provided review focus.
Reviewed flows: REFRESH TABLE -> connector invalidation -> FE lookup invalidation -> engine/row-count invalidation -> journal/replay; query/schema/MVCC -> retained observations -> live handle/view probe -> guarded cleanup -> deferred resource release; named/numeric lookup -> object and ID publication.
Critical checkpoint conclusions:
- Goal and evidence: explicit refresh now reloads lookup membership/identity, ordinary mutations retain identity, and confirmed absence repairs caches. Added tests cover the primary flows; the two inline cases remain gaps in automatic repair.
- Scope and clarity: production changes and shared cache primitives are focused on external metadata refresh and lookup lifecycle; supporting tests exercise those changes.
- Concurrency: SQL queries, row-count/cache workers, refresh/DDL/events and replay can overlap. Checked catalog monitor, stripes, object/registry publication gates and index ordering. Remote probes and resource release are outside the guarded commit; the second finding identifies an incomplete names/object publication boundary. No additional reachable deadlock was established.
- Lifecycle: exact registration/generation checks reject refreshed or replaced catalog/database/table state. Deferred cleanup preserves replacement ownership, drains on failure and clears its shared ThreadLocal; current connector loader policy keeps that context parent-owned. No new thread or static initialization dependency was found.
- Configuration: no configuration item is added. Existing name-miss refresh, case modes and independent expiry/TTL settings are relevant to the findings.
- Compatibility: the journal boolean is additive, absent old fields default false, and the metadata-refresh generation is transient. No plugin SPI signature or FE-BE wire/storage format changes were found.
- Thrift/protobuf: no definitions changed.
- Parallel paths: reviewed explicit/ordinary/event/post-DDL refresh, warm/cold database/table replay, named/numeric lookup, modes 0/1/2 and mappings, ten connector implementations, views/system tables, MVCC and session bypass. Automatic cleanup differs from the broader explicit-refresh path as described inline.
- Conditional checks: initialization, catalog identity, exact membership, registration, generation and session-bypass guards match concrete replacement/stale-response cases. The live view check protects healthy SDK views.
- Test coverage: inspected cache, ES/Paimon, FE refresh/lookup/missing-table tests and Hive regression. Failure propagation, failed cleanup, replacement objects/databases/catalogs, metadata-only refresh, numeric publication and latch-controlled races are covered in source; historical-name reuse and the second finding's interleaving are not.
- Test results: the added Hive output is consistent with its ordered listings and schema statements by inspection. Runtime correctness and generation provenance were not independently verified.
- Observability: existing cache metrics and refresh/error messages retain relevant catalog/database/table context; ES/Paimon now propagate contextual failures. No separate observability defect was established.
- Persistence and failover: explicit lookup invalidation is logged and honored by warm/cold replay; ordinary committed-mutation logging remains before fallible local refresh. No additional persistence discrepancy was found.
- Writes and crashes: this changes local cache invalidation, not remote transaction commit protocols. Reviewed failure cleanup and retained ownership; the two findings concern stale identity/metadata, without an additional durability issue established.
- FE-BE variables: none added.
- Performance: observations use retained entries and bounded local guards, physical cleanup is deferred, and broader explicit case-mode invalidation is intentional. No additional substantiated performance issue was found.
- Other issues and limits: the final missed-issue recheck covered the complete changed-file set and all risk/candidate dispositions. No unresolved candidate or unreviewed changed path remains. This was static review only: no builds, unit/regression tests or BE execution were run.
Review converged in two rounds on this unchanged finding set. Please address both inline P2 findings.
Review converged after 2 round(s).
| ExternalDatabase<?> database = getDb(); | ||
| Optional<Runnable> observation = database.observeTableLookup(this, () -> { | ||
| try { | ||
| connector.invalidateTable(db.getRemoteName(), getRemoteName()); |
There was a problem hiding this comment.
[P2] Clear historical spellings during automatic missing-table repair
With lower_case_table_names=2, warm Foo, externally replace it with FOO, and let a names-only refresh discover FOO (for example, a name-miss lookup for a newly created sibling). The old Foo object/schema and connector entries remain cached while foo resolves to FOO. If FOO is externally replaced by a new Foo, an ordinary query through FOO gets an empty handle and runs this callback, which invalidates only FOO; observeTableLookup also removes only that object/ID. The next names reload maps foo to Foo and can reuse the first incarnation's FE object/schema and Iceberg table/snapshot caches, serving stale metadata/data after automatic repair. Mode 1 can retain historical connector keys here too. Apply case-aware domain invalidation to automatic cleanup while preserving observation fencing, and cover the historical-spelling reuse sequence. This is a separate instance from the earlier explicit-REFRESH finding: this automatic callback never runs that now-broadened refresh path.
| != extCatalog) { | ||
| return; | ||
| } | ||
| database.runIfCurrent(() -> membership.invalidateIfCurrent(() -> |
There was a problem hiding this comment.
[P2] Fence object loads until missing-table names are removed
An ExternalRowCountCache worker can pass getTableNullable(long) initialization and capture the table name, then pause. Here the inner object.invalidateIfCurrent removes the object and releases its stripe/gates before the outer membership.invalidateIfCurrent finally invalidates names. If the worker resumes in that gap, it acquires a fresh table-publication lease and buildTableForInit accepts the table from the still-current names; its capability check can then block on the catalog monitor until this cleanup finishes. It subsequently publishes a replacement object and ID because the later names invalidation belongs to another cache owner and does not fence that lease. The dropped table is therefore cached/indexed again after confirmed-missing repair; subsequent observations skip cleanup once names no longer include it. Keep object publication fenced through retirement of the names snapshot, and add a deterministic numeric-lookup interleaving test.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28734 ms |
TPC-DS: Total hot run time: 150809 ms |
ClickBench: Total hot run time: 24.22 s |
FE Regression Coverage ReportIncrement line coverage |
2 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Problem Summary: External table refresh invalidated connector metadata but retained the FE table-name snapshot and object/ID lookup. After a remote drop or same-name recreation,
SHOW TABLESand lookups could continue using the old identity. ExplicitREFRESH TABLEnow invalidates that lookup state and replays the same decision on follower FEs. Case-folding modes invalidate the database lookup domain to cover old remote spellings; ordinary content mutations retain their narrower invalidation and do not reload table names.An ordinary base-table connector-handle lookup that definitively reports a missing table now also removes the matching FE names/object/ID state and table metadata. The lookup captures exact retained cache registrations before remote I/O and rechecks the catalog, database, names snapshot, table object, and publication generation before committing cleanup. An old missing response cannot remove a refreshed or recreated table. Timeouts and other connector failures propagate without triggering missing-table cleanup. Cleanup is conservative when the matching names snapshot is already cold or the session bypasses shared schema caches.
Logical invalidation happens inside the guarded commit; physical removal and resource-release callbacks run synchronously after the enclosing locks are released. Detached registrations keep cleanup tied to old entries, including when new values are published before cleanup completes. Numeric lookups publish their ID alongside the table object, and newly reloaded objects have an update timestamp before schema loading.
Release note
External table refresh reloads cached table names and lookup identity. A live lookup confirming that a remote table was dropped also clears matching stale lookup state automatically.
Check List (For Author)
test_hive_external_lookup_refresh, generated output and comparison against a local FE connected to real Hive Metastore. Covers explicit refresh, drop/recreate, sibling retention, and automatic cleanup after a missing lookup../build.sh --fe -j4, including Checkstyle.Validation scope: FE unit tests and Hive metadata regression; no BE data-execution validation. The first query that observes a missing table can retain the existing schema-cache error message; this change repairs subsequent lookup/listing state.