github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4045802732
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -484,14 +489,30 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
}
public void invalidateDb(long catalogId, String dbName) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ } finally {
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ rowCountCache.invalidateDb(catalogId,
Util.genIdByName(catalog.getName(), dbName));
+ }
+ }
}
public void invalidateTable(long catalogId, String dbName, String
tableName) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateTable",
- () -> cache.invalidateTable(catalogId, dbName, tableName)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateTable",
+ () -> cache.invalidateTable(catalogId, dbName,
tableName)));
+ } finally {
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ rowCountCache.invalidateTable(catalogId,
Review 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).
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -379,9 +380,13 @@ private Map<String, String>
runtimeEffectiveCacheProperties(
}
public void invalidateCatalog(long catalogId) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateCatalog",
- () -> cache.invalidateCatalogEntries(catalogId)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateCatalog",
+ () -> cache.invalidateCatalogEntries(catalogId)));
+ } finally {
+ rowCountCache.invalidateCatalog(catalogId);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -484,14 +489,30 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
}
public void invalidateDb(long catalogId, String dbName) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ } finally {
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ rowCountCache.invalidateDb(catalogId,
Util.genIdByName(catalog.getName(), dbName));
+ }
+ }
}
public void invalidateTable(long catalogId, String dbName, String
tableName) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateTable",
- () -> cache.invalidateTable(catalogId, dbName, tableName)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateTable",
+ () -> cache.invalidateTable(catalogId, dbName,
tableName)));
+ } finally {
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ rowCountCache.invalidateTable(catalogId,
+ Util.genIdByName(catalog.getName(), dbName),
+ Util.genIdByName(catalog.getName(), dbName,
tableName));
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalRowCountCache.java:
##########
@@ -122,10 +126,16 @@ static Optional<Long> loadRowCount(RowCountKey
rowCountKey, boolean fillMetaCach
public long getCachedRowCount(long catalogId, long dbId, long tableId,
boolean fillMetaCache) {
RowCountKey key = new RowCountKey(catalogId, dbId, tableId);
try {
- CompletableFuture<Optional<Long>> f = fillMetaCache
- ? rowCountCache.get(key, (rowCountKey, executor) ->
CompletableFuture.supplyAsync(
- () -> loadRowCount(rowCountKey, true), executor))
- : rowCountCache.get(key);
+ CompletableFuture<Optional<Long>> f;
+ publicationLock.readLock().lock();
+ try {
+ f = fillMetaCache
+ ? rowCountCache.get(key, (rowCountKey, executor) ->
CompletableFuture.supplyAsync(
+ () -> loadRowCount(rowCountKey, true),
executor))
+ : rowCountCache.get(key);
+ } finally {
+ publicationLock.readLock().unlock();
Review Comment:
[P1] Fence refresh completion, not only refresh start. This cache enables
`refreshAfterWrite`, and [Caffeine 2.9.3's
`refreshIfNeeded`](https://github.com/ben-manes/caffeine/blob/v2.9.3/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java#L1112-L1165)
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/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -484,14 +489,30 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
}
public void invalidateDb(long catalogId, String dbName) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ } finally {
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ rowCountCache.invalidateDb(catalogId,
Util.genIdByName(catalog.getName(), dbName));
Review 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.
--
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]
