924060929 commented on PR #63612: URL: https://github.com/apache/doris/pull/63612#issuecomment-6633157192
<!-- pr63612-local-review-report:1c6f079d88f84c0c1f4681623b2d8048ba474adf --> ### Local code review report — PR #63612: Replay cache invalidation | | | |---|---| | PR | https://github.com/apache/doris/pull/63612 | | Reviewed commit | `1c6f079d88f84c0c1f4681623b2d8048ba474adf` (reviewed local candidate; GitHub PR ref remains b119905) on pushed fork branch `924060929/incubator-doris:fix/sql-cache-replay-invalidate` | | Diff range | `ec7f806fa3907f9be936cbaa789eb0a20476d41c...1c6f079d88f84c0c1f4681623b2d8048ba474adf`; merge base `c0274bfb4accd3f013b320704d55919f7ae46c2e`; target ahead: 2 | | Scope | 30 files, +1634/-184, 5 commits; all 2,899 prepared diff lines read | | Publication scope | The pushed fork tip is the reviewed commit above. The GitHub PR head API currently reports `b1199058a6e2b66bfaf3a924e4503b4e0f8e85d6`. This report is bound to the reviewed commit and is not a pipeline PASS receipt for a different SHA. | | Verdict | **APPROVE for the reviewed commit** | | Rounds | 2 of max 3; converged (verdict stable since round 1) | | Builds on | d4d166c, f56231c and b119905, all 2026-11-04; 24 dismissals inherited/revalidated, changed premises below | #### Background and result Follower and observer FEs serve queries while replaying the original DML/DDL journals. A cache miss can plan old dependencies, release planner locks and execute while replay invalidates cached entries, then publish its old context. SQL cache hits directly return cached data and do not republish that context. This head removes the executing-context index and planner registration. Every production statement captures process-local metadata revisions for its own table IDs/names, database and catalog. Both FE/BE publication and lookup share table/data/view/catalog validation with revision checks before and after it. Publication temporarily installs a fresh statement/connector scope and always closes/restores it. A physical late put remains unusable at lookup. Ordinary visible writes retain partition-selective reuse. Rename/replacement notification now follows name-map mutation while carrying old identities. Two read-only review rounds found no remaining accepted issue: Blocker 0 / Major 0 / Minor 0 / Nit 0; regression findings 0. This is a local review of the pushed fork tip. The GitHub PR head is still older at the recorded read; no current PR-head CI or qualified pipeline PASS is claimed. #### Previous findings and implementation counterchecks F-01 (legacy internal CREATE nullable identity) and F-02 (warm external DB refresh boundary) remain fixed. F-03 (late obsolete SQL publication) is handled by scoped dependency revisions and the shared validator, replacing the b119905 live-context registry. Fix-phase self-review additionally corrected fresh connector scope selection/closure and moved rename/replace notification after mutation. These are fixed code paths, not remaining findings. #### Critical Checkpoints | Checkpoint | Conclusion | |---|---| | Goal and proof | Requested registry removal and shared validation implemented; fix-phase preset UT: 21 classes / 276 cases / zero failures/errors/skips. | | Minimality and abstraction | One revision owner and shared dependency validator; existing Env fanout preserved. No opcode-specific cache copies; original-name overload supports post-mutation notification. | | Concurrency and locks | Atomic scoped cells, snapshot capture fence and lookup recheck cover invalidation/publication races. Bounded native R protects HashMap schema reads; new R scopes release before cloud IO. | | Lifecycle and static init | Snapshot strongly retains revision cells; owner weakly retains cells, never contexts. Temporary connector scope closes explicitly on all exits, statement restores in finally. No executor/static initialization change. | | Configuration | No new config; manager revision state survives result-cache container replacement; tested. | | Compatibility and definitions | Original journals/payloads only; no thrift/proto field or persisted revision. Legacy embedded CREATE retained. | | Parallel paths | FE/BE publication and hit, table/view/db/catalog/all, internal/external, native/cloud, loaded/cold refresh, swap/non-swap checked. | | Conditions | Cold missing scopes, unsupported expressions/tables, temporary-table behavior and DATA/METADATA distinction have concrete paths. | | Test coverage | Real serialized replay, both publishers, forced late put, no-notification data change, positive partition controls, first binding, external branch, scope cleanup, mid-validation invalidation and failure isolation. | | Expected results | Four schema-bearing fallback-key assertions match fixtures; regression output rows match suite SQL. No new generated output edited. | | Observability | Per-family Throwable warnings retained; failed publication validation logs query ID and stack. No new hot INFO log. | | Persistence/replay/failover | Invalidation tied to existing mutation owners, not a second durable log. VISIBLE follows partition then table updates. Local revisions vanish with in-memory caches after restart. | | Data atomicity | No write/commit protocol change; an old query can finish its snapshot but cannot advertise invalid dependencies as a reusable current cache. | | FE-BE variables | No new transport fields; existing data/schema cache key retained. | | Performance | Metadata notification no longer scans active queries; dependency maps no longer COW-copy each insertion. Publication pays one dependency validation; no benchmark claim. DATA reuse unchanged. | | Other scope | 30/30 changed files covered; no unrelated source changes, no agents per user override. | #### Response to focus All requested areas checked. R1-R7 in the risk ledger resolve revision lifetime/capture, fresh scopes, event timing and old names, native/cloud locks, DATA and late puts, all replay/cache families and validation/test fidelity. No unresolved candidate or new important finding remains. The earlier scope-control attempt is explicitly marked inconclusive due to TableSnapshot identity-based mocking; it is not presented as negative proof. Corrected tests match selector content and pass with positive controls. #### Considered and Dismissed Twenty-four earlier dismissals are carried forward with revalidation, revising D-07/D-14 and replacing D-18..D-22's registry premises. Six new counterchecks follow. No issue is downgraded because of trigger rarity; no severity-challenge agent is used under the explicit user override. | ID | Concern | Evidence | Disposition | |---|---|---|---| | D-01 | Companion journal window | [SchemaChangeJobV2.java:859](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java#L859) | Existing live/replay completion owns notification; no new opcode or companion record. | | D-02 | Late obsolete-schema publication | [NereidsSqlCacheManager.java:610](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L610) | Schema/base-index checks and BE schema signature retain partitioned and fallback protection. Metadata-only changes additionally use scoped revisions. | | D-03 | COMMITTED eviction | [DatabaseTransactionMgr.java:2685](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java#L2685) | DATA follows partition then table VISIBLE updates; COMMITTED has no new notification. | | D-04 | Other caches on ordinary DML | [OlapTable.java:147](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java#L147) | Native sorted ranges validate table data version; SimpleAgg validates exact version/time. No ordinary-write freshness bypass established. | | D-05 | Pending SimpleAgg future survives eviction | [SimpleAggCacheMgr.java:328](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/stats/SimpleAggCacheMgr.java#L328) | Inherited Caffeine 3.2.4 artifact proof: keySet.removeIf removes the underlying pending mapping. Dependency and call path unchanged; artifact not reopened. | | D-06 | MV reverse lock or external IO | [MTMVRelationManager.java:98](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java#L98) | New fanout resolves local MV targets then takes existing leaf MV locks; generation protects builds outside locks. No reverse cycle demonstrated. | | D-07 | Old rename/replacement identity | [Alter.java:860](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java#L860) | Revised from prior runs: notification is after mutation while retaining original names and both table IDs. Swap and non-swap notification states pass. | | D-08 | Row-count final fence | [ExternalMetaCacheMgr.java:311](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java#L311) | Common DB notification stays in metadata finally; final row-count invalidation follows even when engine invalidation fails. | | D-09 | Removed catalog cannot be looked up | [CatalogMgr.java:200](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java#L200) | Saved old catalog name is notified after manager locks; rename/drop cleanup does not require looking the old object up again. | | D-10 | Sorted UT repair bypasses production | [NereidsSortedPartitionsCacheManagerExternalTest.java:531](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/test/java/org/apache/doris/common/cache/NereidsSortedPartitionsCacheManagerExternalTest.java#L531) | CALLS_REAL_METHODS reaches real Env fanout and real sorted cache. Actual eviction assertions remain; new SQL-manager fixture dependency is supplied. | | D-11 | Native DATA adds Cloud replay IO | [CloudGlobalTransactionMgr.java:2819](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java#L2819) | Native table versions are local; Cloud transaction-state replay has a distinct unsupported owner. No new native-to-MS replay call. | | D-12 | External catalog property reuse | [NereidsSqlCacheManager.java:784](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L784) | Publication and lookup both compare the existing complete catalog property signature. | | D-13 | Warm catalog reset bypasses callback | [ExternalMetaCacheMgr.java:293](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java#L293) | Inherited synchronous database removal/reset listener meets the moved common DB callback. Loaded/cold x failure controls retained. | | D-14 | MV builder crosses name mutation | [StatementContext.java:1552](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java#L1552) | Related base tables are collected and locked in ID order; name mutation holds W through post-mutation generation invalidation. Existing cache publication checks generation. | | D-15 | Function journals need SQL eviction | [ExpressionAnalyzer.java:84](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/ExpressionAnalyzer.java#L84) | UDF/AliasUdf marks unsupported expressions; cache admission rejects those queries, so reusable function-dependent SQL entry premise is false. | | D-16 | Overwrite task journal changes data | [InsertOverwriteManager.java:393](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/insertoverwrite/InsertOverwriteManager.java#L393) | Task ADD/DROP/CANCEL is bookkeeping; VISIBLE and partition-replacement owners perform actual data changes and notify. | | D-17 | Sorted ranges include query pruning | [OlapTable.java:142](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java#L142) | Native cache stores the full partition item map; external tables use frozen per-query partition views. No empty-pruned result is shared as full topology. | | D-18 | Missing executing-context registration | [StatementContext.java:472](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java#L472) | Prior registry argument is superseded: the only production SqlCacheContext constructor receives a shared revision snapshot at statement creation; no registration is required. | | D-19 | Final check races put | [NereidsSqlCacheManager.java:495](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L495) | Physical late put is possible, but lookup rechecks the same revisions/dependencies. Forced late-put cases reject it. | | D-20 | Weak tracking loses live revision or leaks contexts | [SqlCacheMetadataVersions.java:34](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/SqlCacheMetadataVersions.java#L34) | Snapshot strongly owns each captured cell; owner has only weak values and no context references, no TTL/capacity eviction. Unused cells can collect without losing a live dependency. | | D-21 | Concurrent active dependency map scans | [NereidsSqlCacheManager.java:173](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L173) | Only published entries are scanned. Binding mutates query-owned maps before publication; metadata notifications update atomic cells without reading active maps. COW copies removed. | | D-22 | Config rebuild revives stale values | [NereidsSqlCacheManager.java:223](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L223) | Revision owner belongs to manager and survives result-cache replacement. Copied stale entries still fail shared validation; namespace/config tests pass. | | D-23 | DATA or BE key regression | [SqlCacheContext.java:742](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/SqlCacheContext.java#L742) | DATA does not advance metadata revisions; affected partition data revalidates while unrelated partitions remain reusable. Existing schema fallback signature retained. | | D-24 | Weakened golden or mock tests | [OlapTableSqlCacheVersionTest.java:176](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/test/java/org/apache/doris/nereids/OlapTableSqlCacheVersionTest.java#L176) | Four expected fallback strings match the fixture schema/base index. Actual eviction tests and both real publisher paths remain. | | D-25 | First dependency attaches after refresh | [SqlCacheMetadataVersions.java:77](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/SqlCacheMetadataVersions.java#L77) | Statement-start event sequence is checked on both sides of capture, even for a previously cold scope; repeated capture cannot adopt a newer revision. | | D-26 | Fresh FE map still uses old connector table | [NereidsSqlCacheManager.java:476](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L476) | Temporary current statement gives real ConnectorSessionBuilder a fresh scope as well as fresh MVCC maps; old query pin/scope is retained and restored. | | D-27 | Unlocked native schema maps or MS IO under new locks | [OlapTableSqlCacheVersion.java:154](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/OlapTableSqlCacheVersion.java#L154) | Bounded R protects schema/partition reference reads, is released before cloud partition IO, then reacquired to check metadata and object identity; table R precedes cloud version R. | | D-28 | Validation failure leaks scope or fails query | [NereidsSqlCacheManager.java:482](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java#L482) | Inner finally closes temporary scope on success/failure and under deferred Flight cleanup; outer finally restores the executing statement; Throwable skips publication only. | | D-29 | New statement enters after premature notification | [Env.java:5902](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java#L5902) | Rename/replace callbacks moved after completed map mutation, retaining original names and IDs; notification-time probes cover renamed and both replacement modes. | | D-30 | FE publication still holds planner locks | [NereidsPlanner.java:1208](https://github.com/924060929/incubator-doris/blob/1c6f079d88f84c0c1f4681623b2d8048ba474adf/fe/fe-core/src/main/java/org/apache/doris/nereids/NereidsPlanner.java#L1208) | Premise false: FE publication is in handleQueryInFe after plan returns; BE publication is after EOS. planWithLock finally already released planner resources. | #### Convergence and limits | Round | Owner/focus | New B/M/Minor/Nit | Coverage | Verdict | |---|---|---|---|---| | 1 | Main: entire authoritative diff, prior runs/threads, seven risks | 0/0/0/0 | 30/30 | APPROVE | | 2 | Main: reverse admission/hit flow, revisions/lifetime, scopes, locks, replay/data/cache-family counterchecks | 0/0/0/0 | 30/30 | APPROVE | - Full prepared diff and all five commits read; required repo checklist and eight AGENTS fully read this/prior unchanged context. Existing threads and all three prior run ledgers/meta read. The per-file coverage ledger has zero gaps in both rounds. - Fix-phase verification before this review: run-fe-ut.sh exit 0 / BUILD SUCCESS / 07:18, 21 classes / 276 cases / zero failures/errors/skips; Checkstyle zero violations. XML reports and execution logs were retained locally. No build/test/source edit in review phase. No latest-head live multi-FE/BE or Cloud run, nor benchmark; earlier multi-FE generated output belongs to earlier recorded heads. - All source changes are committed. Untracked local review and execution artifacts are excluded from the diff. - Fork tip read-back confirms 1c6f079d88f84c0c1f4681623b2d8048ba474adf. At the final recorded PR read, REST/GraphQL and refs/pull/63612/head still show b119905 despite the fork branch being advanced. This operational synchronization issue is separate from code approval. The initial check reported branch mismatch and ahead:1; that commit is pushed, and this local document explicitly reviews it. Do not treat this as an approval of an updated GitHub PR head until synchronization is verified. - Review runtime: `gpt-6.1-sol`, effort `xhigh`, verified from the original review turn. Solo review per the author's explicit instruction. This is the requested review report; the automated PASS receipt remains subject to exact PR-head matching. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
