branch-4.1: [fix](meta cache) Invalidate external row count cache after metadata changes #64160 - #68196
branch-4.1: [fix](meta cache) Invalidate external row count cache after metadata changes #64160#68196924060929 wants to merge 1 commit into
Conversation
|
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-only review of exact head 947f086dd99ed57f8bb6e4bb655b7347f4cef312 against base 8d2018bdc72c989d885814215644294478041aa5. I found five distinct correctness gaps in the new row-count invalidation behavior; details are inline.
Coverage included both changed files and their DDL, HMS event, replay/failover, catalog lifecycle, partition-DML, name-mapping, and async-publication call chains. I also checked configuration bounds, error paths, lock ordering/performance, compatibility, and existing test oracles. This is an FE-only cache change with no wire/storage-format change. The bounded O(N) scan and catalog property-update path were adjudicated as non-findings. No user focus was supplied. Per the review contract, I did not run builds or tests and made no source edits. The third and final review round converged with no new valuable findings.
| cache, catalogId, "invalidateCatalog", | ||
| () -> cache.invalidateCatalogEntries(catalogId))); | ||
| } finally { | ||
| rowCountCache.invalidateCatalog(catalogId); |
There was a problem hiding this comment.
[P1] Invalidate row counts on every whole-catalog retirement. This call is only reached by invalidateCatalog, while DROP/replay use removeCatalogPermanently and rename uses removeCatalog; neither clears rowCountCache. Because external table IDs are deterministic from names and RowCountKey.equals compares only tableId, recreating the same catalog name can return the dropped catalog's future (and rename-back has the same reuse). Please add a final catalog-scope row-count barrier to those retirement paths.
| } finally { | ||
| CatalogIf<?> catalog = getCatalog(catalogId); | ||
| if (catalog != null) { | ||
| rowCountCache.invalidateDb(catalogId, Util.genIdByName(catalog.getName(), dbName)); |
There was a problem hiding this comment.
[P1] Derive this ID from the resolved database, not the caller's spelling. With lower_case_database_names=1/2, DROP DATABASE can resolve a canonical DB and then pass the original DDL spelling through afterDropDb/replay to this line; in mode 2, HMS events also lowercase a mixed-case stored name. The resulting hash differs from the DB ID carried by row-count keys, so the scan removes nothing and a same-name recreation reuses stale counts. Please capture/pass the canonical numeric identity before removal and cover both case-insensitive DDL/replay and a mixed-case mode-2 event.
| } finally { | ||
| CatalogIf<?> catalog = getCatalog(catalogId); | ||
| if (catalog != null) { | ||
| rowCountCache.invalidateTable(catalogId, |
There was a problem hiding this comment.
[P1] Do not make point eviction depend on a resident ExternalTable. The table-object cache defaults to 1,000 entries while this cache retains 100,000, so the row count can outlive its object. In that state an HMS DROP/RENAME event cannot rebuild the already-removed table and returns from unregisterExternalTable; cold replayRefreshTable similarly returns before reaching this call. A later same-name CREATE regenerates the same table ID and reuses the stale future. Please carry/canonicalize the identity so event and replay paths evict even when the metadata object is absent (or conservatively evict the DB scope).
| if (catalog != null) { | ||
| rowCountCache.invalidateTable(catalogId, | ||
| Util.genIdByName(catalog.getName(), dbName), | ||
| Util.genIdByName(catalog.getName(), dbName, tableName)); |
There was a problem hiding this comment.
[P1] Also evict the table row count after partition-scoped mutations. A partitioned Hive insert refreshes only affected partition caches, and HMS add/drop/alter-partition events likewise call only partition helpers, so none reaches this new table eviction. HMSExternalTable can derive its row count from the partition/file list, which means a successful commit or event continues serving the pre-mutation cached count. Please add a final table row-count fence to the partition mutation and replay paths.
| () -> loadRowCount(rowCountKey, true), executor)) | ||
| : rowCountCache.get(key); | ||
| } finally { | ||
| publicationLock.readLock().unlock(); |
There was a problem hiding this comment.
[P1] Fence refresh completion, not only refresh start. This cache enables refreshAfterWrite, and Caffeine 2.9.3's refreshIfNeeded completes with compute(key, ...) and returns the refreshed value when currentValue == null. Therefore an expired read can start refresh under this lock, unlock here, be invalidated under the write lock, and then have the pre-invalidation refresh reinsert the key. Please add an invalidation generation/epoch (or another completion-time ownership check) and a deterministic refresh-start -> invalidate -> refresh-complete test.
FE UT Coverage ReportIncrement line coverage |
2 participants
Cherry-picked from #64160