Uh oh!
There was an error while loading. Please reload this page.
[fix](meta cache) Invalidate external row count cache after metadata changes - #64160
Conversation
hello-stephen
commented
Jun 5, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
foxtail463
commented
Jun 5, 2026
run buildall |
hello-stephen
commented
Jun 5, 2026
TPC-H: Total hot run time: 29357 ms |
hello-stephen
commented
Jun 5, 2026
TPC-DS: Total hot run time: 169748 ms |
hello-stephen
commented
Jun 5, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 5, 2026
FE Regression Coverage ReportIncrement line coverage |
| cacheRegistry.resetForTest(caches); | ||
| } | ||
| void replaceRowCountCacheForTest(ExternalRowCountCache rowCountCache) { |
There was a problem hiding this comment.
change name to setRowCountCache, and add comment to explain this function only used for test
morrySnow
commented
Jun 8, 2026
/review |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
There was a problem hiding this comment.
Review result: no additional blocking issues found in this pass. I did not duplicate the existing inline thread about renaming/commenting replaceRowCountCacheForTest.
Critical checkpoint conclusions:
- Goal/test: The PR invalidates ExternalRowCountCache on external metadata changes. The main refresh, partition add/drop, Hive insert, Iceberg action, drop, truncate, db, and catalog invalidation paths were traced; added unit tests cover table invalidation and manager wiring, though they do not exhaustively cover every metadata operation.
- Scope: The change is small and focused, mostly replacing table-cache invalidation with table-level invalidation that also clears row counts, plus targeted partition-row-count invalidation.
- Concurrency: The new cache invalidation uses Caffeine concurrent cache APIs; no new locks or lock-order changes were introduced. I did not find heavy work added under FE catalog/database/table locks.
- Lifecycle/static initialization: No new static/global lifecycle dependency was introduced. The test-only cache replacement is already covered by an existing review thread.
- Configuration/compatibility: No new config, persistence format, thrift protocol, storage format, or mixed-version compatibility issue was introduced.
- Parallel paths: I checked refresh table/db/catalog, refresh partitions, Hive partition add/drop, Hive insert replay, Iceberg actions, metadata drop/truncate hooks, and engine-specific invalidation callers. The relevant content-changing paths appear covered; engine-specific property-cache removals do not appear to be content changes requiring row-count invalidation.
- Conditional checks/error handling: No new suspicious defensive checks or silently ignored Status-like results were introduced in the FE Java paths.
- Observability: Existing logging remains sufficient for this cache invalidation change; no new metric appears necessary.
- Transaction/persistence/data writes: Hive insert writes the existing refresh edit log after local invalidation, and replay invalidates row count on partition/full refresh. I did not find a master-failover replay gap from this patch.
- Performance: Catalog/db invalidation removes matching cache keys by scanning the row-count cache map. Given these operations are metadata refresh/removal paths rather than query hot paths and the cache capacity is bounded, this is acceptable.
- Tests: New unit tests validate table invalidation reload behavior and ExternalMetaCacheMgr table invalidation wiring. I did not run tests in this review runner.
User focus points: No additional user-provided review focus was specified, so no extra focused issue was found.
foxtail463
commented
Jun 9, 2026
run buildall |
hello-stephen
commented
Jun 9, 2026
TPC-H: Total hot run time: 28934 ms |
hello-stephen
commented
Jun 9, 2026
TPC-DS: Total hot run time: 168174 ms |
hello-stephen
commented
Jun 9, 2026
FE Regression Coverage ReportIncrement line coverage |
foxtail463
commented
Jun 10, 2026
run nonConcurrent |
hello-stephen
commented
Jun 10, 2026
FE Regression Coverage ReportIncrement line coverage |
morrySnow
commented
Jun 17, 2026
/review |
There was a problem hiding this comment.
Automated review completed for PR 64160. I did not find blocking issues in the changed code.
Checkpoint conclusions:
- Goal/test: The PR targets stale external row-count cache after external metadata changes. The changed paths now invalidate row-count cache for catalog/db/table invalidation, HMS partition add/drop/refresh/replay, Hive insert commit, and Iceberg table-changing actions. Added FE unit coverage verifies table invalidation and reload behavior.
- Scope/focus: The changes are small and focused on unifying metadata cache invalidation with row-count cache invalidation.
- Concurrency/locking: No new locks or shared mutable structures outside the existing Caffeine cache paths. Catalog/db invalidation scans the row-count cache via concurrent cache views; table invalidation uses the existing synchronous invalidation API.
- Lifecycle/static/config/compatibility: No new static lifecycle, config item, storage format, protocol, or FE/BE variable compatibility concern found.
- Parallel paths: I checked refresh table/db/catalog, partition refresh replay, HMS add/drop partition events, Hive insert commit, and Iceberg snapshot/manifest actions. The changed data-mutating paths are covered; engine-specific cache config removals are not data-change paths.
- Transaction/persistence: Hive insert and refresh replay paths propagate row-count invalidation consistently with the existing edit-log refresh model.
- Testing/results: New tests are FE unit tests only; no regression .out files are involved. I did not run tests in this review runner.
- Observability/performance: No new hot-path logging. The only broad scan is bounded row-count cache invalidation on catalog/db invalidation, not a query hot path.
- Security: The PR does not touch authentication, authorization, network boundary, or tenancy-sensitive behavior.
User focus response: .code-review.KuD14R/review_focus.txt contained no additional focus points; no extra focus-specific issue was found.
foxtail463
commented
Aug 17, 2026
run buildall |
foxtail463
commented
Aug 17, 2026
/review |
foxtail463
commented
Aug 17, 2026
run performance |
db93496 to
4ba3cf6Comparefoxtail463
commented
Aug 17, 2026
run buildall |
1 similar comment
foxtail463
commented
Aug 17, 2026
run buildall |
hello-stephen
commented
Aug 17, 2026
TPC-H: Total hot run time: 17193 ms |
hello-stephen
commented
Aug 17, 2026
TPC-DS: Total hot run time: 91896 ms |
foxtail463
commented
Aug 18, 2026
run feut |
hello-stephen
commented
Aug 18, 2026
FE UT Coverage ReportIncrement line coverage |
foxtail463
commented
Aug 18, 2026
run buildall |
hello-stephen
commented
Aug 18, 2026
TPC-H: Total hot run time: 17189 ms |
hello-stephen
commented
Aug 18, 2026
TPC-DS: Total hot run time: 80143 ms |
hello-stephen
commented
Aug 18, 2026
FE Regression Coverage ReportIncrement line coverage |
foxtail463
commented
Aug 18, 2026
run cloud_p0 |
hello-stephen
commented
Aug 18, 2026
FE Regression Coverage ReportIncrement line coverage |
924060929
commented
Aug 19, 2026
/review |
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. |
924060929
commented
Aug 20, 2026
/review |
There was a problem hiding this comment.
Automated review summary
No new valuable findings beyond the existing live review threads. This is a comment-only review with zero inline comments; two full review rounds converged, and every candidate was independently validated, deduplicated, or dismissed with base/head evidence.
Critical checkpoints:
- Goal, scope, and tests: The 29-file change consistently targets external row-count invalidation across refresh, DDL, event, replay, action, rewrite, and insert paths. The added tests cover exact/scoped invalidation, mapped identities, concurrency, replay, and post-commit fencing; their negative space was reviewed as well.
- Concurrency and lifecycle: Row-count mapping publication/removal is serialized, connector -> engine -> row-count ordering is preserved, and passive object eviction remains separate from explicit structural invalidation. No new publication, teardown, or exception-ordering defect was established.
- Identity, configuration, and compatibility: Modes 0/1/2, cross-database rename, hot/cold/uninitialized replay, property resets, and current/legacy log shapes were traced. Potential concerns either predate this PR or are already covered by live compatibility/identity threads, so they were not duplicated.
- Transactions, persistence, and data writes: Single actions, distributed rewrite, and normal inserts place the newly added final fence after the owning commit and before later fallible result work. Failure/no-op paths avoid the new final fence; no new durability or retry-visible regression was found.
- Parallel paths and conditions: Coordinator/follower, mapped/unmapped, hot/cold, exact/case-insensitive, empty/non-empty, and single/distributed branches were checked. No distinct head-only issue remained.
- Performance, observability, and variables: The known O(N) invalidation concerns are already live-thread fenced. No new FE/BE variable propagation, logging, or configuration defect was introduced.
- Test execution: Per the review-run prompt, no builds or tests were run; conclusions are from the authoritative diff, production control-flow tracing, bundled tests, and live-thread deduplication.
User focus: No additional review focus was provided.
Review status: complete and converged in Round 2; zero new inline comments.
Uh oh!
There was an error while loading. Please reload this page.
Problem Summary: PR apache#64160 added row-count cache invalidation and a test-only engine cache replacement hook after this branch diverged. Merging current master conflicted with the scoped metadata cache test setup and left the old registry-based test hook incompatible with the new cacheTypes framework. Solution: Preserve the row-count cache implementation and invalidation ordering from apache#64160. Resolve RefreshManagerTest against FeMetaCacheEntry and migrate replaceEngineCachesForTest to register replacement engines and aliases through the new cacheTypes model. Tests: - 116 focused FE unit tests passed - 104 connector cache unit tests passed - Maven Checkstyle reactor passed with 0 violations Issue Number: closeapache#66633 Related PR: apache#64160
### What problem does this PR solve? Issue Number: closeapache#64159 Related PR: apache#64160 Problem Summary: Incremental metadata events published the ID-to-name mapping before the matching database or table object was committed to the shared cache. A concurrent lookup by ID could therefore load and publish a different object in that gap, which was then replaced and cleaned up by the event update. Publish the ID mapping and object inside one guarded cache commit, and fence loads that overlap either side of the auxiliary action so they can not publish stale objects. ### Release note None ### Check List (For Author) - Test: Unit Test - ScopedMetaCacheConcurrencyTest - FeMetaCacheEntryTest - ExternalCatalogTest - ExternalDatabaseTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: closeapache#64159 Related PR: apache#64160 Problem Summary: Incremental metadata events published the ID-to-name mapping before the matching database or table object was committed to the shared cache. A concurrent lookup by ID could therefore load and publish a different object in that gap, which was then replaced and cleaned up by the event update. Publish the ID mapping and object inside one guarded cache commit, fence overlapping loads, and make the object-and-ID step run only once when the outer name-cache CAS retries so a newer object is never replaced by replaying an already completed event update. ### Release note None ### Check List (For Author) - Test: Unit Test - ScopedMetaCacheConcurrencyTest - FeMetaCacheEntryTest - ExternalCatalogTest - ExternalDatabaseTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
Problem Summary
External table row counts are cached separately from metadata caches. Some metadata mutation and replay paths did not invalidate row counts, allowing stale values to surviverefreshes,
DML/DDL operations, cold-cache replay, and table renames until cache expiration, which could affect statistics estimation.
Solution
Add scoped invalidation and concurrent-load fencing to ExternalRowCountCache, and integrate it with unified metadata invalidation paths. Cover external metadata mutations and replay,
including cold deterministic identities and both sides of a rename. Add real-cache tests to verify eviction and reload behavior.