github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4089921042
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -226,8 +252,18 @@ public void replayRefreshTable(ExternalObjectLog log) {
// Partition-level cache invalidation, only for hive
catalog
HiveExternalMetaCache cache =
Env.getCurrentEnv().getExtMetaCacheMgr()
.hive(catalog.getId());
- cache.refreshAffectedPartitionsCache(
- (HMSExternalTable) table.get(), modifiedPartNames,
newPartNames);
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateRowCountCache(table.get());
Review Comment:
[P1] Fence the row count before acquiring the Hive cache group. This replay
handles an already-committed insert, but `hive(catalogId)` can lazily
initialize the group and throw before this new fence. The outer
`replayRefreshSafely` then swallows the failure and advances without retry,
leaving the follower's pre-insert row count resident; the full-table fallback
is never reached either. Move the held-table fence ahead of the `hive(...)`
lookup and add a throwing-acquisition replay test. This is distinct from the
existing selective-refresh thread, which starts after acquisition succeeds.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -147,8 +151,15 @@ public void resetMetaToUninitialized(boolean
invalidateEngineCache) {
}
}
if (invalidateEngineCache) {
+ // Route through the typed overload: connector-specific caches
(for example Paimon's
+ // table loader) are keyed by the database object and are not
fully covered by the
+ // name-based scan in invalidateDb(long, String).
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(this);
}
+ if (invalidateRowCountCache) {
+ Env.getCurrentEnv().getExtMetaCacheMgr()
Review Comment:
[P1] Keep the DB row-count fence failure-safe. Warm `REFRESH DATABASE`
replay reaches the typed engine invalidation above before this new row-count
call, and the route can throw in production—for example, Paimon's
`invalidateDatabase` converts an SDK failure into `CacheException`. That skips
the fence; `replayRefreshSafely` then swallows the error and advances the
committed record, so rebuilt tables can reuse pre-refresh counts. Put the DB
row-count invalidation in a `finally` around the routed invalidation and add a
throwing warm-replay regression. This is distinct from the existing cold-DB
replay thread.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -158,6 +158,11 @@ private RemovedCatalog removeCatalog(long catalogId) {
}
String catalogName = catalog.getName();
Env.getCurrentEnv().getRefreshManager().removeFromRefreshMap(catalogId);
+ // Publish the row-count fence while the mapping write lock still
excludes a same-name
+ // CREATE. External table ids are deterministic from catalog/DB/table
names and RowCountKey
+ // equality uses only tableId, so a replacement admitted before this
scan could observe the
+ // retired catalog's resident row-count entries.
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateRowCountCache(catalogId);
Review Comment:
[P1] Fence after removing the old mappings, while still holding this write
lock. With the fence here, a reader can register a row-count load immediately
afterward and `StatisticsUtil.findTable` can still resolve the old catalog
through the lock-free maps before lines 166-167 run. That load was admitted
after the only fence, so it can publish after DROP and alias a same-name
replacement via the table-ID-only `RowCountKey`. This is the converse of the
existing post-unlock replacement thread: remove both mappings first, then fence
before unlocking, and add a latching late-old-load/recreate test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/HiveInsertExecutor.java:
##########
@@ -83,17 +83,37 @@ protected void doBeforeCommit() throws UserException {
protected void doAfterCommit() throws DdlException {
HMSExternalTable hmsTable = (HMSExternalTable) table;
+ // The transaction is already committed. Fence the row-count cache by
the held table
+ // identity before any fallible cache work (including
isPartitionedTable reinitialization),
+ // so an evicted or partially reloaded table cannot retain the
pre-insert count.
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateRowCountCache(hmsTable);
Review Comment:
[P1] Pair this opening fence with a completion fence after successful
selective refresh. A row-count load admitted just after this call is not marked
by the invalidation; before the selective refresh below clears the
partition/file caches it can compute the old value through
`getRowCountFromFileList`, then publish and retain it after success because
only the failure/full-table paths call `invalidateTableCache`. Mirror the
closing fence in follower replay and live partition-event selective paths, and
add a latching regression. Existing threads cover pre-fence or failing refresh
work, not this successful post-fence admission window.
--
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]
