github-actions[bot] commented on code in PR #64160:
URL: https://github.com/apache/doris/pull/64160#discussion_r3750720671
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -1312,13 +1312,13 @@ private static ConnectorColumnPath
toConnectorPath(ColumnPath columnPath) {
* {@code createForRefreshTable} editlog (LOCAL names, replay-neutral) and
a {@code refreshTableInternal}
* (re-resolving the local cached table by its REMOTE names, mirroring
legacy
* {@code IcebergMetadataOps.refreshTable}). {@code refreshTableInternal}
is the single source of truth for
- * the cache work ({@code unsetObjectCreated} + {@code setUpdateTime} +
{@code invalidateTableCache} + the
+ * the cache work ({@code unsetObjectCreated} + {@code setUpdateTime} +
{@code invalidateTable} + the
* connector-side per-table cache drop), so it must NOT be re-inlined here.
*/
protected void afterExternalDdl(ExternalTable externalTable, long
updateTime) {
Env.getCurrentEnv().getEditLog().logRefreshExternalTable(
- ExternalObjectLog.createForRefreshTable(getId(),
- externalTable.getDbName(), externalTable.getName(),
updateTime));
+ ExternalObjectLog.createForRefreshTable(getId(),
externalTable.getDbId(),
Review Comment:
**[P1] Carry truncate IDs through cold follower replay**
This exact-ID propagation also needs to cover the parallel external truncate
edit log. The current external `TruncateTableInfo` constructor leaves
`dbId`/`tblId` as 0, and the plugin replay method wraps `refreshTableInternal`
in nested cache-only `ifPresent` calls. A follower can have the table object
evicted while its independent row-count and connector caches remain hot, so
replay becomes a no-op and later reuses the same deterministic table ID with
the pre-truncate count/snapshot. Please journal the resolved IDs and add a
cold-table replay fallback (with conservative connector invalidation when the
remote name is unavailable), plus a follower test for that state.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalRowCountCache.java:
##########
@@ -162,4 +162,18 @@ public long getCachedRowCountIfPresent(long catalogId,
long dbId, long tableId)
return -1;
}
+ // Catalog/db invalidation is O(N): row-count keys are numeric ids, and
Caffeine
+ // does not support prefix invalidation by catalog or database id.
+ void invalidateCatalog(long catalogId) {
Review Comment:
**[P1] Fence in-flight publication during bulk invalidation**
These DB/catalog `removeIf` scans only remove mappings visible to their weak
iterator. Caffeine invokes the async loader inside `computeIfAbsent`, while
Doris's loader schedules `supplyAsync` before returning its future; the worker
can therefore capture the old connector snapshot while the future is still
unpublished. Connector invalidation and this scan can both finish while the key
is absent, after which the stale future is inserted and retained. This survives
merely moving connector invalidation first (and also affects already
connector-first DB drop/event paths). Please add catalog/DB invalidation
generations or equivalent admission/publication serialization, and a latch test
that pauses future publication across the bulk invalidation.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -253,8 +272,9 @@ public void replayRefreshTable(ExternalObjectLog log) {
log.debugForRefreshTable());
return;
}
- Env.getCurrentEnv().getExtMetaCacheMgr()
- .invalidateTable(catalog.getId(), db.get().getFullName(),
localTableName);
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateTable(
+ log.getCatalogId(), dbIdForInvalidation,
db.get().getFullName(),
+ tableIdForInvalidation, localTableName);
if (catalog instanceof PluginDrivenExternalCatalog) {
Review Comment:
**[P1] Evict row counts after connector metadata**
`invalidateTable(table)` now removes the row-count entry before the
following connector-cache invalidation. A concurrent miss can start in that
window and compute from the still-cached connector snapshot (Iceberg statistics
resolve through its table cache), then publish the old count after
`connector.invalidateTable` has returned; the connector hook does not evict
that new row-count future. The same ordering exists for DB/catalog refresh.
Please make row-count eviction the final publication step after connector
invalidation (or repeat it afterward); for DB/catalog scopes this must be
paired with the separate publication fence noted on `ExternalRowCountCache`.
Add a latch-based test that opens this inter-call 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]
