github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r3910799318
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1266,16 +1272,14 @@ private String getLocalDatabaseName(String dbName,
boolean isReplay) {
finalName = dbName.toLowerCase();
} else if (mode == 2) {
// Mode 2: Case-insensitive comparison
- finalName = lowerCaseToDatabaseName.get(dbName.toLowerCase());
- if (finalName == null && !isReplay) {
- // Refresh database list and try again
+ if (!isReplay) {
try {
- getFilteredDatabaseNames();
- finalName =
lowerCaseToDatabaseName.get(dbName.toLowerCase());
+ metaCache.listNames();
Review Comment:
[P1] Keep the routing-map clear atomic with names invalidation
`resetToUninitialized()` clears `lowerCaseToDatabaseName` under the catalog
monitor, releases that monitor, and only afterward invalidates `metaCache`
through `onRefreshCache()`. A concurrent `getDbNullable()` can reinitialize in
that gap; this call then hits the still-complete old names entry without
republishing the cleared map, so a following differently-cased mode-2 lookup
misses. This is separate from the stale-loader thread because no load runs in
this interleaving. Please order the map clear and names invalidation under the
same initialization fence (or republish one atomic snapshot), and add a paused
reset test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -432,15 +437,10 @@ public DatabaseProperty getDbProperties() {
public boolean isTableExist(String tableName) {
String remoteTblName = tableName;
if (this.isTableNamesCaseInsensitive()) {
+ metaCache.listNames();
Review Comment:
[P1] Initialize the database before reading its names cache
On a cold object-cache miss, a database returned by
`ExternalCatalog.getDbNullable()` is newly constructed but has not run its own
`makeSureInitialized()`, so its `metaCache` is still null.
`CreateTableCommand.targetTableExists()` immediately calls the generic
`DatabaseIf.isTableExist()` path; with `lower_case_table_names=2`, this new
call therefore throws before reaching the connector. Please establish database
initialization (and handle initialization failure) before using `metaCache`,
and cover a cold mode-2 existence probe.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +130,110 @@ public MetaCache(String name,
maxSize,
true,
null);
- namesCache = namesCacheFactory.buildCache(namesCacheLoader, executor);
+ namesCache = namesCacheFactory.buildCache();
// Use sync removal listener to prevent deadlock (removal listener
calls invalidateAll)
// NOTE: This cache should NOT use refreshAfterWrite, as it would
become synchronous
metaObjCache =
objCacheFactory.buildCacheWithSyncRemovalListener(metaObjCacheLoader,
removalListener);
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ return
getNames().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ private List<Pair<String, String>> getNames() {
+ while (true) {
+ NamesCacheValue value = namesCache.getIfPresent("");
+ if (value == null || !value.complete) {
+ value = loadNames(false);
+ if (value == null) {
Review Comment:
[P1] Bound retries when metadata keeps mutating
Every `updateCache()` or `invalidate()` advances the cache-wide generation,
and any advance during a slow enumeration makes `loadNames()` return null to
this unconditional loop. With even one unrelated HMS event spanning each
enumeration, a foreground `listNames()` can issue remote loads forever and
never return; the old same-key Caffeine ordering let the load finish before
applying the mutation. Please replay a bounded mutation journal (including
deletions) into a completed snapshot, or otherwise provide a bounded
convergence/fallback policy, and test several consecutive generation changes.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -509,26 +509,21 @@ private String getLocalTableName(String tableName,
boolean isReplay) {
finalName = tableName.toLowerCase();
}
if (this.isTableNamesCaseInsensitive()) {
+ if (!isReplay) {
+ metaCache.listNames();
Review Comment:
[P2] Use the known event entry before requiring a full listing
On a cold mode-2 cache, `registerTable()` has already installed the table
object and its lowercase routing entry, but
`HMSExternalDatabase.registerTable()` immediately calls `getTableNullable()` to
apply the event update time. This unconditional `listNames()` sees the
incomplete entry and forces a full remote enumeration first; if that
enumeration fails, the authoritative event itself fails even though the
requested table is already cached. The base path consulted the specific routing
entry before listing. Please allow a known incremental hit here and enumerate
only on a miss, with a cold registration/loader-failure test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +130,110 @@ public MetaCache(String name,
maxSize,
true,
null);
- namesCache = namesCacheFactory.buildCache(namesCacheLoader, executor);
+ namesCache = namesCacheFactory.buildCache();
// Use sync removal listener to prevent deadlock (removal listener
calls invalidateAll)
// NOTE: This cache should NOT use refreshAfterWrite, as it would
become synchronous
metaObjCache =
objCacheFactory.buildCacheWithSyncRemovalListener(metaObjCacheLoader,
removalListener);
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ return
getNames().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ private List<Pair<String, String>> getNames() {
+ while (true) {
+ NamesCacheValue value = namesCache.getIfPresent("");
+ if (value == null || !value.complete) {
+ value = loadNames(false);
+ if (value == null) {
+ continue;
+ }
+ }
+ boolean current;
+ synchronized (namesMutationLock) {
+ current = value.generation == namesGeneration.get();
+ }
+ if (current) {
+ scheduleNamesRefresh(value);
+ return value.names;
+ }
+ }
+ }
+
+ private NamesCacheValue loadNames(boolean forceRefresh) {
+ synchronized (namesLoadLock) {
Review Comment:
[P1] Let the post-invalidation generation load independently
`namesLoadLock` is held across connector I/O for automatic refreshes. If one
starts before `invalidateAll()`, invalidation advances the generation and
returns, but the next `listNames()` blocks here behind that obsolete refresh;
if its RPC hangs, refresh can never restore metadata access. The base Caffeine
refresh ran asynchronously, so an invalidated synchronous miss could load
independently. Please deduplicate loads per generation (or otherwise
cancel/abandon the obsolete generation), and test a second lookup while a
pre-invalidation automatic refresh remains paused.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +130,110 @@ public MetaCache(String name,
maxSize,
true,
null);
- namesCache = namesCacheFactory.buildCache(namesCacheLoader, executor);
+ namesCache = namesCacheFactory.buildCache();
// Use sync removal listener to prevent deadlock (removal listener
calls invalidateAll)
// NOTE: This cache should NOT use refreshAfterWrite, as it would
become synchronous
metaObjCache =
objCacheFactory.buildCacheWithSyncRemovalListener(metaObjCacheLoader,
removalListener);
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ return
getNames().stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ private List<Pair<String, String>> getNames() {
+ while (true) {
+ NamesCacheValue value = namesCache.getIfPresent("");
+ if (value == null || !value.complete) {
+ value = loadNames(false);
+ if (value == null) {
+ continue;
+ }
+ }
+ boolean current;
+ synchronized (namesMutationLock) {
+ current = value.generation == namesGeneration.get();
+ }
+ if (current) {
+ scheduleNamesRefresh(value);
+ return value.names;
+ }
+ }
+ }
+
+ private NamesCacheValue loadNames(boolean forceRefresh) {
+ synchronized (namesLoadLock) {
+ List<Pair<String, String>> incompleteNames;
+ long loadGeneration;
+ synchronized (namesMutationLock) {
+ NamesCacheValue cached = namesCache.getIfPresent("");
+ if (!forceRefresh && cached != null && cached.complete
+ && cached.generation == namesGeneration.get()) {
+ return cached;
+ }
+ incompleteNames = cached != null && !cached.complete
+ ? Lists.newArrayList(cached.names) :
Lists.newArrayList();
+ loadGeneration = namesGeneration.get();
+ }
+ List<Pair<String, String>> loadedNames;
+ try {
+ loadedNames =
Objects.requireNonNull(namesCacheLoader.load(""));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new CompletionException(e);
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new CompletionException(e);
+ }
+ synchronized (namesMutationLock) {
+ if (loadGeneration != namesGeneration.get() || loadGeneration
< minimumLoadGeneration) {
+ return null;
+ }
+ List<Pair<String, String>> names =
Lists.newArrayList(loadedNames);
+ incompleteNames.forEach(pair ->
NameMutation.update(pair.key(), pair.value()).apply(names));
+ NamesCacheValue value = new
NamesCacheValue(namesGeneration.get(), names, true);
+ namesCache.put("", value);
+ publishNames(value);
+ return value;
+ }
+ }
+ }
+
+ private void scheduleNamesRefresh(NamesCacheValue value) {
+ if (System.nanoTime() - value.writeNanos < namesRefreshAfterWriteNanos
+ || !namesRefreshRunning.compareAndSet(false, true)) {
+ return;
+ }
+ startNamesRefresh();
+ }
+
+ private void startNamesRefresh() {
+ try {
+ namesRefreshExecutor.execute(() -> {
+ try {
+ loadNames(true);
Review Comment:
[P2] Contain refresh failures inside the shared executor task
`loadNames(true)` can throw a runtime or wrapped checked exception, but this
raw `execute` task only has `finally`, so a normal connector refresh failure
escapes and terminates a worker in the shared `commonRefreshExecutor`. Since
the old value remains refresh-eligible, repeated accesses during an outage can
repeatedly kill and replace workers shared by the legacy catalog/database
caches and engine metadata caches. Please catch and log loader failures inside
the runnable while clearing the flag and retaining the old value, and test that
a failed refresh is retried without an uncaught task failure.
--
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]
