github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4131397957
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/event/CreateDatabaseEvent.java:
##########
@@ -55,7 +55,8 @@ protected static List<MetastoreEvent>
getEvents(NotificationEvent event,
protected void process() throws MetastoreNotificationException {
try {
logInfo("catalogName:[{}],dbName:[{}]", catalogName, dbName);
-
Env.getCurrentEnv().getCatalogMgr().registerExternalDatabaseFromEvent(dbName,
catalogName);
+
Env.getCurrentEnv().getCatalogMgr().registerExternalDatabaseFromEvent(
+ event == null ? dbName : event.getDbName(), catalogName);
Review Comment:
[P1] Normalize the local name for mixed-case HMS CREATE events. With
`lower_case_database_names=1`, an event for remote `Foo` now passes `Foo` to
`registerDatabaseFromEvent`, which caches the database and its ID as `Foo`;
ordinary discovery and DROP lookup use `foo`. A later DROP of `Foo` invalidates
`foo` and leaves the event-created `Foo` entry in the names/object cache, so a
dropped database remains visible until refresh. Keep the original spelling as
the remote name, but derive the event local name and ID with the same mode-1
normalization as database listing; cover CREATE and rename followed by DROP.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -520,9 +597,51 @@ private void invalidateLanceTableAccess(long catalogId) {
public void invalidatePartitions(long catalogId,
String dbName, String tableName, List<String> partitions) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidatePartitions",
- () -> cache.invalidatePartitions(catalogId, dbName, tableName,
partitions)));
+ Runnable rowCountFence = () ->
rowCountCache.invalidateCatalog(catalogId);
+ try {
+ rowCountFence = resolveTableRowCountFence(catalogId, dbName,
getCachedDb(catalogId, dbName), tableName);
+ rowCountFence.run();
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidatePartitions",
+ () -> cache.invalidatePartitions(catalogId, dbName,
tableName, partitions)));
+ } finally {
+ rowCountFence.run();
+ }
+ }
+
+ private Runnable resolveTableRowCountFence(long catalogId, String dbName,
+ Optional<ExternalDatabase<? extends ExternalTable>> db, String
tableName) {
+ if (db.isPresent()) {
+ Optional<? extends ExternalTable> table =
db.get().getTableForReplay(tableName);
+ if (table.isPresent()) {
+ long tableCatalogId = table.get().getCatalog().getId();
+ long tableDbId = table.get().getDb().getId();
+ long tableId = table.get().getId();
+ return () -> rowCountCache.invalidateTable(tableCatalogId,
tableDbId, tableId);
+ }
+ long dbId = db.get().getId();
Review Comment:
[P2] Use the retained table name to fence a cold table by ID. When
`getTableForReplay` misses after ordinary object-cache eviction, this branch
fences the entire DB even though its canonical table name can still be
resolved. Every ADD/DROP/ALTER PARTITION event for a cold table then scans up
to 100,000 row-count entries under the global publication write lock; with over
1,000 tables in a DB, this can stall unrelated row-count readers and event
catch-up. Derive the deterministic table ID from the retained local name and
use the exact table fence, keeping DB scope for a genuinely lost name mapping.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -120,16 +120,25 @@ public void setRemoteName(String remoteName) {
}
public void resetMetaToUninitialized() {
- resetMetaToUninitialized(true);
+ resetMetaToUninitialized(true, true);
}
public void resetMetaToUninitialized(boolean invalidateEngineCache) {
+ resetMetaToUninitialized(invalidateEngineCache, invalidateEngineCache);
+ }
+
+ public void resetMetaToUninitialized(boolean invalidateEngineCache,
boolean invalidateRowCountCache) {
if (LOG.isDebugEnabled()) {
LOG.debug("resetToUninitialized db name {}, id {}, isInitializing:
{}, initialized: {}",
this.name, this.id, isInitializing, initialized, new
Exception());
}
MetaCache<T> cacheToInvalidate = null;
Runnable objectInvalidation = null;
+ if (invalidateRowCountCache) {
Review Comment:
[P1] Fence Lance row counts before retiring table access. Both leader and
replay REFRESH DATABASE call `invalidateLanceTableAccess` before reaching this
opening fence; follower REFRESH TABLE replay also evicts access before its
later table or DB fence. After a remote namespace rebind, a query in either gap
can resolve the replacement dataset through fresh Lance access while
`ExternalTable.getRowCount` still returns the old completed count. Publish the
corresponding row-count fence before each access-cache invalidation and retain
the completion fences; latch a query between those steps.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -714,26 +711,42 @@ public void resetToUninitialized(boolean invalidCache) {
*/
public void onRefreshCache(boolean invalidCache) {
setLastUpdateTime(System.currentTimeMillis());
- refreshMetaCacheOnly(invalidCache);
- if (invalidCache) {
- Env.getCurrentEnv().getExtMetaCacheMgr().invalidateCatalog(id);
+ try {
+ refreshMetaCacheOnly(invalidCache);
+ } finally {
+ if (invalidCache) {
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateCatalog(id);
+ }
}
}
/**
* Refresh meta cache only (database level cache), without invalidating
catalog level cache.
*/
private void refreshMetaCacheOnly(boolean invalidCache) {
- if (metaCache != null) {
- // A catalog-wide engine invalidation below supersedes every
database invalidation.
- // The legacy cache uses a synchronous removal listener, so this
thread-local scope
- // prevents one full SDK-cache scan per cached database without
affecting concurrent
- // expiry callbacks on other threads.
- invalidateEngineCacheOnDatabaseRemoval.set(!invalidCache);
- try {
- metaCache.invalidateAll();
- } finally {
- invalidateEngineCacheOnDatabaseRemoval.remove();
+ Runnable objectInvalidation;
+ ExternalMetaCacheMgr cacheMgr =
Env.getCurrentEnv().getExtMetaCacheMgr();
+ synchronized (this) {
+ if (metaCache == null) {
+ return;
+ }
+ // Publish the names/object generation transition together, before
allowing another
+ // catalog initialization. The old cache removal callbacks can be
much slower.
+ cacheMgr.invalidateRowCountCache(id);
Review Comment:
[P2] Move the row-count sweep out of the catalog monitor. This opening fence
calls `invalidateCatalog`, which scans up to 100,000 row-count entries and all
in-flight loads under a global write lock while `synchronized(this)` is held.
Ordinary `getDbNames` and `getDbNullable` planning must enter that same
monitor, so REFRESH CATALOG still blocks them for the full sweep even though
its DB/table removal callbacks now run outside it. Keep a fast generation fence
under the monitor, then retire matching entries without holding the catalog
monitor; latch the sweep itself in a concurrency test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -332,8 +426,15 @@ public void refreshPartitions(String catalogName, String
dbName, String tableNam
ExternalTable externalTable = (ExternalTable) table;
HiveExternalMetaCache cache =
Env.getCurrentEnv().getExtMetaCacheMgr().hive(externalTable.getCatalog().getId());
- for (String partitionName : partitionNames) {
- cache.invalidatePartitionCache(externalTable, partitionName);
+ try {
Review Comment:
[P2] Include Hive cache acquisition in the closing-fence `try`. This ALTER
PARTITION path opens a row-count fence before table lookup, but
`hive(catalogId)` can throw during lazy engine preparation before the new
`finally` is entered. A concurrent count load admitted after the opening fence
can then publish the pre-event value and remain cached until retry or expiry.
Put acquisition and other fallible work after the opening fence inside an outer
`try/finally` that always closes it; ADD PARTITION has the same gap around its
Hive/schema lookup. Extend the acquisition-failure test to require two fences.
--
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]
