github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4056140135
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -128,7 +128,7 @@ public void resetMetaToUninitialized() {
metaCache.invalidateAll();
}
}
-
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(extCatalog.getId(),
getFullName());
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(extCatalog.getId(),
getId(), getFullName());
Review Comment:
[P1] Avoid a global row-count scan for every DB removed by a catalog reset.
The catalog DB cache can hold 1,000 objects and its removal listener runs
synchronously; `invalidateAll()` therefore calls this method once per DB, and
each call now scans both global row-count registries under
`publicationLock.writeLock()`. With up to 100,000 row-count entries (including
other catalogs), a scheduled/full catalog refresh can do tens of millions of
checks while repeatedly blocking every row-count reader, before the
catalog-scope scan runs as well. Please bulk-fence the catalog once while
suppressing per-DB scans during catalog reset, or maintain scope indexes.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -669,15 +736,32 @@ public ExternalRowCountCache getRowCountCache() {
}
public void invalidateTableCache(ExternalTable dorisTable) {
- invalidateTable(dorisTable.getCatalog().getId(),
- dorisTable.getDbName(),
- dorisTable.getName());
+ long catalogId = dorisTable.getCatalog().getId();
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateTableCache",
+ () -> cache.invalidateTable(catalogId,
dorisTable.getDbName(), dorisTable.getName())));
+ } finally {
+ invalidateRowCountCache(dorisTable);
Review Comment:
[P1] Make this row-count fence reachable from cold TRUNCATE replay. A
follower dispatches `TruncateTableInfo` to
`HiveMetadataOps.afterTruncateTable`, but that callback uses cache-only
DB/table lookups and silently does nothing when the table object has been
evicted. The table-object cache holds 1,000 entries with no removal listener
while row counts retain up to 100,000, so the old count can survive; TRUNCATE
preserves the table ID, and subsequent reads reuse it. Add a name-based
fallback for an absent table and a cold follower replay test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalRowCountCache.java:
##########
@@ -87,6 +103,59 @@ protected Optional<Long> doLoad(RowCountKey rowCountKey) {
}
}
+ private final class InvalidationAwareLoader implements
AsyncCacheLoader<RowCountKey, Optional<Long>> {
+ private final RowCountCacheLoader delegate;
+
+ private InvalidationAwareLoader(RowCountCacheLoader delegate) {
+ this.delegate = delegate;
+ }
+
+ @Override
+ public CompletableFuture<Optional<Long>> asyncLoad(RowCountKey key,
Executor executor) {
+ return loadWithInvalidationFence(key, executor, () ->
delegate.doLoad(key));
+ }
+ }
+
+ private static final class LoadFence {
+ private boolean invalidated;
+ }
+
+ private CompletableFuture<Optional<Long>> loadWithInvalidationFence(
+ RowCountKey key, Executor executor, Supplier<Optional<Long>>
loader) {
+ LoadFence fence = new LoadFence();
+ publicationLock.readLock().lock();
+ try {
+ inFlightLoads.compute(key, (ignored, fences) -> {
+ Set<LoadFence> currentFences = fences == null ?
ConcurrentHashMap.newKeySet() : fences;
+ currentFences.add(fence);
+ return currentFences;
+ });
+ } finally {
+ publicationLock.readLock().unlock();
+ }
+
+ CompletableFuture<Optional<Long>> publishedFuture = new
CompletableFuture<>();
+ CompletableFuture.supplyAsync(loader, executor).whenComplete((value,
throwable) -> {
Review Comment:
[P1] Remove this fence when task submission is rejected. The fence is
inserted before `supplyAsync`, but the only removal is in the future's
`whenComplete`; if `executor.execute` throws, no future exists and the fence
remains forever. The production row-count pool has a bounded queue and a
zero-second rejection policy while this cache permits 100,000 entries, so a
load burst followed by retries can grow `inFlightLoads` without bound and make
every scope invalidation scan retained junk. Please clean up this exact fence
on synchronous submission failure and add a rejecting-executor test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalRowCountCache.java:
##########
@@ -87,6 +103,59 @@ protected Optional<Long> doLoad(RowCountKey rowCountKey) {
}
}
+ private final class InvalidationAwareLoader implements
AsyncCacheLoader<RowCountKey, Optional<Long>> {
+ private final RowCountCacheLoader delegate;
+
+ private InvalidationAwareLoader(RowCountCacheLoader delegate) {
+ this.delegate = delegate;
+ }
+
+ @Override
+ public CompletableFuture<Optional<Long>> asyncLoad(RowCountKey key,
Executor executor) {
+ return loadWithInvalidationFence(key, executor, () ->
delegate.doLoad(key));
+ }
+ }
+
+ private static final class LoadFence {
+ private boolean invalidated;
+ }
+
+ private CompletableFuture<Optional<Long>> loadWithInvalidationFence(
+ RowCountKey key, Executor executor, Supplier<Optional<Long>>
loader) {
+ LoadFence fence = new LoadFence();
+ publicationLock.readLock().lock();
+ try {
+ inFlightLoads.compute(key, (ignored, fences) -> {
Review Comment:
[P1] Track the full scope on each in-flight fence. This map uses
`RowCountKey.equals`, which compares only `tableId`, but catalog/db
invalidation later filters the stored key's `catalogId`/`dbId`. If a same-name
catalog is dropped and recreated while an old load remains in flight, a
new-generation fence joins the equal old map entry without replacing its
representative key; a later invalidation of the new catalog misses that fence,
and Caffeine can republish its refresh after eviction. Key this registry by
full identity (or filter scope stored on each fence) and cover same-table-ID
catalog recreation.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -452,6 +458,7 @@ public void removeCatalogPermanently(long catalogId) {
}
}
} finally {
+ rowCountCache.invalidateCatalog(catalogId);
Review Comment:
[P1] Apply this fence to tentative-property rollback as well. Legacy
validators publish `newProps` before `checkProperties`, while lock-free catalog
lookup and lazy initialization do not take this lifecycle stripe. A concurrent
row-count load can therefore initialize the catalog/client and start against
the temporary endpoint. If validation fails, `rollbackCatalogProperties` below
only restores `CatalogProperty` and routed engine groups; it neither resets
candidate-built catalog/DB/table state nor fences row counts, so the rejected
target's client/count can remain under unchanged IDs. Reset that legacy state
and add the catalog-scope completion fence during rollback, with a latching
regression test.
--
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]
