morrySnow commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3540970048
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -432,15 +445,11 @@ public DatabaseProperty getDbProperties() {
public boolean isTableExist(String tableName) {
Review Comment:
**CONFIRMED: Mode-2 case-insensitive replay may fail after names-only
invalidation.**
After `resetMetaCacheNames()` invalidates the `tableNames` snapshot,
case-insensitive replay lookups fail because the lower-case index was moved
into the Caffeine-cached `NameCacheValue`. The old `lowerCaseToTableName`
ConcurrentHashMap survived `resetMetaCacheNames()` independently. The exact-key
check above (`tables.getIfPresent(tblName)`) only helps with exact-case matches
— in mode-2, the object cache key is the original remote case, so `"mytable"`
won't match `"MyTable"`.
**Fix:** Maintain the lower-case index separately from the Caffeine-cached
snapshot, or add a fallback that scans cached object keys when the names
snapshot is cold during replay.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -228,42 +307,125 @@ private V getWithManualLoad(K key, Function<K, V>
loadFunction) {
return value;
}
- long generation = invalidateGeneration.get();
- V loaded = loadAndTrack(key, loadFunction);
- if (generation != invalidateGeneration.get()) {
- return loaded;
+ long generation;
+ // Snapshot generation only after re-checking the cache under the
publication lock so
+ // public mutations cannot slip between the miss observation and
the captured version.
+ synchronized (publishLock(key)) {
+ value = data.asMap().get(key);
+ if (value != null) {
+ return value;
+ }
+ generation = generationOf(key);
}
-
- // Keep null results uncached so manual miss load matches
LoadingCache null-return behavior.
+ V loaded = loadAndTrack(key, loadFunction);
if (loaded == null) {
return null;
}
- // Leave a narrow hook for tests to pause exactly before the cache
put race window.
Review Comment:
**CONFIRMED: Lost load deduplication — concurrent `get(same-key)` triggers N
duplicate remote loads.**
The old code held `synchronized(loadLock(key))` through the entire
`loadAndTrack` call, deduplicating concurrent misses for the same key. The new
code releases `publishLock` before `loadAndTrack` here and never acquires a
load-deduplication lock. The `loadLocks` array is now dead code — allocated and
initialized but never locked.
**Scenario:** N threads call `getDbNullable("same_cold_db")`. Each observes
a miss, releases `publishLock`, and independently calls `loadAndTrack` →
`buildDbForInit` → remote `listDatabaseNames`. For slow HMS, N requests trigger
N remote enumeration calls instead of 1.
**Fix:** Re-acquire `loadLock(key)` through the slow load, or use
`ConcurrentHashMap<Key, CountDownLatch>` to deduplicate in-flight loads.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -895,16 +921,14 @@ protected ExternalDatabase<? extends ExternalTable>
buildDbForInit(String remote
// Because in ut, the database is not created in remote system.
Review Comment:
**Same mode-2 replay regression as the database level.**
`getDbForReplay(String)` fails after `invalidateDatabaseNamesOnly()` because
`resolveDatabaseNameFromSnapshot(... isReplay=true)` returns null. The
exact-key check above provides partial but insufficient protection.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1281,6 +1297,103 @@ private String getLocalDatabaseName(String dbName,
boolean isReplay) {
return finalName;
}
+ private NameCacheValue getDatabaseNamesValue(boolean allowLoad) {
+ if (databaseNames == null) {
+ return null;
+ }
+ return allowLoad ? databaseNames.get("") :
databaseNames.getIfPresent("");
+ }
+
+ // Centralize names-negative-lookup handling so all database lookup paths
share the same config-driven policy.
+ @Nullable
+ private <T> T resolveDatabaseNameFromSnapshot(String lookupName, boolean
isReplay,
+ Function<NameCacheValue, T> resolver) {
+ NameCacheValue cached = getDatabaseNamesValue(false);
+ if (cached == null) {
+ if (isReplay) {
+ return null;
+ }
+ NameCacheValue loaded = getDatabaseNamesValue(true);
+ return loaded == null ? null : resolver.apply(loaded);
+ }
+ T resolved = resolver.apply(cached);
+ if (resolved != null || isReplay ||
!Config.enable_external_meta_cache_name_miss_refresh) {
+ return resolved;
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("refresh database names after hot-snapshot miss,
catalog: {}, lookup: {}",
+ getName(), lookupName);
+ }
+ invalidateDatabaseNamesOnly();
+ NameCacheValue refreshed = getDatabaseNamesValue(true);
+ return refreshed == null ? null : resolver.apply(refreshed);
+ }
+
+ private List<String> listLocalDatabaseNamesFromCache() {
+ NameCacheValue namesValue = java.util.Objects.requireNonNull(
+ getDatabaseNamesValue(true), "database names cache can not be
null");
+ return
namesValue.names().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ @Nullable
+ protected String getRemoteDatabaseName(String localDbName, boolean
isReplay) {
+ // Route local-to-remote resolution through the shared helper so miss
reload stays consistent with lookups.
+ return resolveDatabaseNameFromSnapshot(localDbName, isReplay,
+ namesValue -> namesValue.remoteNameOfLocalName(localDbName));
+ }
+
+ protected final void updateDatabaseCache(String remoteDbName, String
localDbName,
+ ExternalDatabase<? extends ExternalTable> db) {
+ updateDatabaseCache(remoteDbName, localDbName, db, false);
+ }
+
+ private void updateDatabaseCache(String remoteDbName, String localDbName,
+ ExternalDatabase<? extends ExternalTable> db, boolean
forceUpdateCacheState) {
+ buildMetaCache();
+ long dbId = db.getId();
+ // Runtime incremental events only maintain names and object entries
that are already hot. The ID map is a
+ // lightweight lookup index and must always track registered objects
so normal by-ID lookup can load on demand.
+ if (forceUpdateCacheState) {
+ databaseNames.compute("", (ignored, current) ->
+ (current == null ? NameCacheValue.empty() :
current).withName(remoteDbName, localDbName));
+ } else if (databaseNames.getIfPresent("") != null) {
+ databaseNames.compute("", (ignored, current) ->
+ current == null ? null : current.withName(remoteDbName,
localDbName));
+ }
+ if (forceUpdateCacheState || databases.getIfPresent(localDbName) !=
null) {
+ databases.put(localDbName, db);
Review Comment:
**PLAUSIBLE: Race on `getIfPresent`-then-`compute`.** A concurrent
`invalidateAll()` between the `getIfPresent("")` check and `compute("", ...)`
evicts the entry. The `compute` sees `current == null` and returns `null`,
which via `ConcurrentMap.compute` semantics **removes the key** — leaving the
names cache empty. Other concurrent readers observe an empty snapshot,
triggering unnecessary remote reloads.
**Fix:** Use `compute` unconditionally (remove the `getIfPresent` guard),
relying on the `current == null → null` branch to preserve the cold-entry skip
semantics.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -142,35 +201,60 @@ public V getIfPresent(K key) {
}
public void put(K key, V value) {
+ // Public mutations participate in generation control so in-flight
loads cannot overwrite them later.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(value, "value can not be null");
if (!effectiveEnabled) {
return;
}
- data.put(key, value);
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ data.put(key, value);
+ }
+ }
+
+ public V compute(K key, BiFunction<K, V, V> remappingFunction) {
+ // Public compute must also advance the stripe generation before
mutating the cache state.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(remappingFunction, "remappingFunction can not
be null");
+ if (!effectiveEnabled) {
+ return null;
+ }
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ return data.asMap().compute(key, remappingFunction);
+ }
}
public void invalidateKey(K key) {
- invalidateGeneration.incrementAndGet();
- if (data.asMap().remove(key) != null) {
- invalidateCount.incrementAndGet();
+ Objects.requireNonNull(key, "key can not be null");
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ if (data.asMap().remove(key) != null) {
+ invalidateCount.incrementAndGet();
+ }
}
}
public void invalidateIf(Predicate<K> predicate) {
- invalidateGeneration.incrementAndGet();
- data.asMap().keySet().removeIf(key -> {
+ Objects.requireNonNull(predicate, "predicate can not be null");
+ // Cover in-flight manual loads whose keys are still outside the cache
map.
+ bumpAllGenerations();
+ for (K key : data.asMap().keySet()) {
if (predicate.test(key)) {
- invalidateCount.incrementAndGet();
- return true;
+ invalidateKey(key);
}
- return false;
- });
+ }
}
public void invalidateAll() {
- invalidateGeneration.incrementAndGet();
- long size = data.estimatedSize();
- data.invalidateAll();
- invalidateCount.addAndGet(size);
+ // Cover in-flight manual loads whose keys are still outside the cache
map.
Review Comment:
**Double generation bump in `invalidateAll()`.** `bumpAllGenerations()`
(below) increments every stripe, then each `invalidateKey(key)` called in the
loop bumps the same stripe AGAIN — double overcount. For
`max_meta_object_cache_num=1000`, this means 1000 lock acquisitions. Consider a
fast path after `bumpAllGenerations()` that removes keys without the per-key
`bumpGeneration()` call.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -570,17 +671,15 @@ public void unregisterTable(String tableName) {
LOG.debug("unregister table {}.{}", this.name, tableName);
}
setLastUpdateTime(System.currentTimeMillis());
- // check if the table exists in cache, it not, does return
- ExternalTable dorisTable = getTableForReplay(tableName).orElse(null);
+ String localTableName = toLocalTableName(tableName);
+ ExternalTable dorisTable =
getTableForReplay(localTableName).orElse(null);
+ // Always clean local names/object/ID state, even when the object
entry is cold or already evicted.
+ if (isInitialized()) {
+ invalidateTableCache(localTableName);
+ }
Review Comment:
**PLAUSIBLE: `unregisterTable` may use wrong cache key in mode-2.**
`toLocalTableName()` here returns its input unchanged in mode-2. But callers
like `CatalogMgr` pass `table.getName()` — already the local/cached name, which
may differ from the original-remote-case name used as the object cache key. The
old code obtained the table object first via `getTableForReplay(tableName)`,
then used `dorisTable.getName()` for invalidation — always correct.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/NameCacheValue.java:
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.metacache;
+
+import org.apache.doris.common.Pair;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Immutable snapshot of remote/local names and the case-insensitive
remote-name index.
+ */
+public final class NameCacheValue {
+ private final ImmutableList<Pair<String, String>> names;
+ private final ImmutableMap<String, String> lowerCaseToRemoteName;
+
+ private NameCacheValue(List<Pair<String, String>> names) {
+ // Deep-copy each pair so callers cannot mutate the snapshot through
reused Pair instances.
+ this.names = ImmutableList.copyOf(copyPairs(names));
+ // Build the lower-case index with last-write-wins semantics so the
snapshot does not
+ // introduce case-conflict validation beyond what the catalog-aware
loader already enforces.
+ Map<String, String> indexBuilder = new java.util.HashMap<>();
+ for (Pair<String, String> pair : this.names) {
+ indexBuilder.put(pair.key().toLowerCase(Locale.ROOT), pair.key());
+ }
+ lowerCaseToRemoteName = ImmutableMap.copyOf(indexBuilder);
+ }
+
+ public static NameCacheValue of(List<Pair<String, String>> names) {
+ return new NameCacheValue(Objects.requireNonNull(names, "names can not
be null"));
+ }
+
+ public static NameCacheValue empty() {
+ return of(ImmutableList.of());
+ }
+
+ public List<Pair<String, String>> names() {
+ // Return fresh Pair objects even though the list itself is immutable,
so callers cannot
+ // mutate the published snapshot through reused Pair instances.
+ return ImmutableList.copyOf(copyPairs(names));
+ }
+
+ public String remoteNameOfLocalName(String localName) {
+ for (Pair<String, String> pair : names) {
+ if (pair.value().equals(localName)) {
+ return pair.key();
+ }
+ }
Review Comment:
**Efficiency: O(n) linear scans on hot paths.** `remoteNameOfLocalName()`
and `containsLocalName()` (below) scan all name pairs linearly. Called from
`buildDbForInit()`/`buildTableForInit()` on every object load. The constructor
already builds `lowerCaseToRemoteName` — consider also building
`localName→remoteName` and a `Set<String>` for O(1) lookups.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/NameCacheValue.java:
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.metacache;
+
+import org.apache.doris.common.Pair;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Immutable snapshot of remote/local names and the case-insensitive
remote-name index.
+ */
+public final class NameCacheValue {
+ private final ImmutableList<Pair<String, String>> names;
+ private final ImmutableMap<String, String> lowerCaseToRemoteName;
+
+ private NameCacheValue(List<Pair<String, String>> names) {
+ // Deep-copy each pair so callers cannot mutate the snapshot through
reused Pair instances.
+ this.names = ImmutableList.copyOf(copyPairs(names));
+ // Build the lower-case index with last-write-wins semantics so the
snapshot does not
+ // introduce case-conflict validation beyond what the catalog-aware
loader already enforces.
+ Map<String, String> indexBuilder = new java.util.HashMap<>();
+ for (Pair<String, String> pair : this.names) {
+ indexBuilder.put(pair.key().toLowerCase(Locale.ROOT), pair.key());
+ }
+ lowerCaseToRemoteName = ImmutableMap.copyOf(indexBuilder);
+ }
+
+ public static NameCacheValue of(List<Pair<String, String>> names) {
+ return new NameCacheValue(Objects.requireNonNull(names, "names can not
be null"));
+ }
+
+ public static NameCacheValue empty() {
+ return of(ImmutableList.of());
+ }
+
+ public List<Pair<String, String>> names() {
+ // Return fresh Pair objects even though the list itself is immutable,
so callers cannot
+ // mutate the published snapshot through reused Pair instances.
+ return ImmutableList.copyOf(copyPairs(names));
+ }
+
+ public String remoteNameOfLocalName(String localName) {
Review Comment:
**Efficiency: `names()` deep-copies all pairs on every call** via
`copyPairs()` then `ImmutableList.copyOf()`. Called on hot paths like `SHOW
DATABASES`/`SHOW TABLES`. The internal field is already an `ImmutableList` of
copy-constructed Pairs — the second deep copy is redundant for read-only paths.
Consider returning the internal `names` field directly since it's already
immutable.
--
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]
