github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4120456635
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -691,15 +785,55 @@ public void invalidateTableCache(ExternalTable
dorisTable) {
long catalogId = dorisTable.getCatalog().getId();
// Typed table invalidation bypasses the name-based invalidateTable()
entry point, so the
// Lance access-cache retirement that used to happen there has to be
repeated here.
- invalidateLanceTableAccess(catalogId);
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateTable", () ->
cache.invalidateTable(dorisTable)));
+ try {
+ invalidateLanceTableAccess(catalogId);
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateTable", () ->
cache.invalidateTable(dorisTable)));
+ } finally {
+ invalidateRowCountCache(dorisTable);
+ }
if (LOG.isDebugEnabled()) {
LOG.debug("invalid table cache for {}.{} in catalog {}",
dorisTable.getRemoteDbName(),
dorisTable.getRemoteName(),
dorisTable.getCatalog().getName());
}
}
+ /**
+ * Best-effort invalidation for a metadata event that carries the caller's
DB/table spelling.
+ * Resolves canonical local identity, then fences engine caches and row
counts at the narrowest
+ * scope that still covers the event; widens to the canonical database or
catalog scope when the
+ * cached object has already been evicted, so caller spelling can never
miss a canonical key.
+ */
+ public void invalidateTableByNameOrWider(long catalogId, String dbName,
String tableName) {
+ Optional<ExternalDatabase<? extends ExternalTable>> db =
getCachedDb(catalogId, dbName);
+ if (!db.isPresent()) {
+ invalidateCatalog(catalogId);
Review Comment:
[P2] Keep cold known-database events scoped to that database. `getCachedDb`
is cache-only, so ordinary DB-object eviction reaches this `invalidateCatalog`
branch even while `getDbIdentityForReplay` still knows the canonical name and
ID. A single TRUNCATE or whole-table/partition event then evicts unrelated hot
DBs' engine entries and row counts; the new row-count pre-fence has the same
catalog fallback. Resolve the retained canonical identity and invalidate that
DB, using catalog scope only when the mapping is truly lost. Please cover a
cold target DB with an unrelated hot DB primed. This is a separate
event/TRUNCATE path from the already raised follower REFRESH and DROP TABLE
cases.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1247,10 +1273,37 @@ public void unregisterDatabase(String dbName) {
if (LOG.isDebugEnabled()) {
LOG.debug("unregister database [{}]", dbName);
}
- if (isInitialized()) {
- metaCache.invalidate(dbName, Util.genIdByName(name, dbName));
+ if (!isInitialized()) {
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(getId(),
dbName);
+ return;
}
- Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(getId(), dbName);
+ String localDbName = getLocalDatabaseName(dbName, true);
+ if (localDbName == null) {
+ // A mode-2 remote-to-local mapping can disappear (for example
after a names refresh)
+ // while the resident database object survives. The canonical key
is then unknown, so
+ // treat the scope as unknown: retire every cached database object
and flush the engine
+ // caches and row counts catalog-wide instead of evicting the
wrong local key.
+ retireAllDatabaseObjectsWithoutEngineInvalidation();
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateCatalog(getId());
+ return;
+ }
+ metaCache.invalidate(localDbName, Util.genIdByName(name, localDbName));
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(getId(),
localDbName);
Review Comment:
[P2] Preserve the known database ID through this DROP.
`metaCache.invalidate` above removes the DB object and ID mapping, so the new
name-based `invalidateDb` overload cannot recover its ID and falls back to
invalidating every row count in the catalog. That evicts unrelated hot DBs and
scans the global row-count cache for each ordinary DROP DATABASE; for a
resident DB, the removal callback has already fenced this DB. Pass the computed
canonical ID to the explicit-ID overload, and test that another DB's cached
count survives.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -408,12 +408,9 @@ public void afterDropTable(String dbName, String tblName) {
invalidatePaimonCatalogForUnresolvedReplay();
}
} else {
- // The database itself could not be resolved (for example a
mode-2 mapping was lost
- // before replay). Retire any retained legacy database object
first so a same-name
- // recreation cannot reuse its stale nested table-name cache,
then flush the engine
- // group; a failure in either is best-effort so the drop log
is still written.
-
dorisCatalog.retireAllDatabaseObjectsWithoutEngineInvalidation();
- invalidatePaimonCatalogForUnresolvedReplay();
+ // A cold DB with a retained canonical mapping has a narrow
invalidation target.
+ // Only a genuinely lost mapping requires catalog-wide
hidden-object retirement.
+ dorisCatalog.invalidateColdDatabaseForReplay(dbName);
Review Comment:
[P1] Retire Paimon's SDK cache on this cold known-DB path.
`invalidateColdDatabaseForReplay` now routes name-based `invalidateDb`, but
Paimon's name-based invalidation only walks Doris `tableEntry`; if a direct
`getPaimonTable` populated SDK `CachingCatalog` without a Doris entry, it never
calls the SDK database/catalog invalidator. The prior branch flushed that SDK
cache, and the existing `testReplayDropInvalidatesSdkOnlyPaimonCache` primes
exactly this state. After the remote DROP, the cached table handle can remain
visible; cold REFRESH DB/TABLE replay takes the same route. Invalidate the SDK
database scope independently of Doris table entries, or retain the Paimon
catalog fallback, and test the cold DB-object case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -883,15 +892,33 @@ private void
alterExternalCatalogPropsFenced(ExternalCatalog externalCatalog, Ca
Integer[] sec = {metadataRefreshIntervalSec,
metadataRefreshIntervalSec};
Env.getCurrentEnv().getRefreshManager().addToRefreshMap(catalogId,
sec);
}
- externalCatalog.modifyCatalogProps(newProps);
// The commit reset the catalog's execution context and closed its SDK
resources. Cached
// base generations and projections are bound to the replaced context;
retire them now so
// the next statement loads a generation the planning fences accept,
instead of retrying
- // against an unplannable cached generation until managed refresh.
+ // against an unplannable cached generation until managed refresh. The
properties are
+ // published before the reset's throwable cleanup, so retirement must
run either way.
Env currentEnv = Env.getCurrentEnv();
ExternalMetaCacheMgr cacheMgr = currentEnv == null ? null :
currentEnv.getExtMetaCacheMgr();
- if (cacheMgr != null) {
-
cacheMgr.onCatalogOperationalContextChanged(externalCatalog.getId());
+ try {
+ externalCatalog.modifyCatalogProps(newProps);
+ } catch (RuntimeException e) {
+ if (!isReplay) {
+ throw e;
+ }
+ // A follower must not terminate because a local connector cleanup
failed while applying
+ // an already-durable ALTER record. The property publication and
the derived-state
+ // transitions above are failure-safe, so the record is considered
applied.
+ LOG.warn("Failed to complete local cleanup while replaying ALTER
CATALOG for {}: {}",
+ externalCatalog.getName(), e.getMessage(), e);
+ } finally {
+ if (cacheMgr != null) {
+
cacheMgr.onCatalogOperationalContextChanged(externalCatalog.getId());
Review Comment:
[P1] Publish an opening row-count fence before this ALTER exposes the new
metadata generation. `modifyCatalogProps` resets the catalog and swaps its
DB-object cache before this new final `onCatalogOperationalContextChanged`
fence runs. A cold DB has no removal callback, so a concurrent query can
rebuild a table under the new properties and read its old-target completed
count during that interval; the later fence cannot retract the plan. Keep the
completion fence as well, and add a latch test between `retireObjects()` and
this call. The earlier property-commit thread covered a missing final fence;
the explicit REFRESH CATALOG opening-fence thread is a different path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -146,8 +150,20 @@ public void resetMetaToUninitialized(boolean
invalidateEngineCache) {
objectInvalidation.run();
}
}
- if (invalidateEngineCache) {
- Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(this);
+ try {
+ 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);
+ }
+ } finally {
+ if (invalidateRowCountCache) {
+ // Independent of the routed invalidation: a connector cache
failure (for example
+ // Paimon's CacheException) must not skip the row-count fence.
+ Env.getCurrentEnv().getExtMetaCacheMgr()
+ .invalidateRowCountCache(extCatalog.getId(), getId());
Review Comment:
[P1] Fence this database's row counts before swapping its table-object
generation. Warm REFRESH DATABASE calls `resetMetaToUninitialized`, which
installs the new table cache at `retireObjects()` and releases the DB monitor
before the new row-count fence here runs, after routed engine invalidation. A
concurrent query can load the new table generation and reuse the old completed
count during that gap; the later fence cannot retract it. Retain this
completion fence and add a latch test between `retireObjects()` and this call.
The existing DB thread covers a throwing invalidation, while the catalog
refresh thread covers a different reset path.
--
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]
