github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r3996728020
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -421,13 +424,34 @@ private void buildMetaCache() {
OptionalLong.of(Config.external_cache_refresh_time_minutes
* 60L),
Math.max(Config.max_meta_object_cache_num, 1),
ignored -> getFilteredDatabaseNames(),
+ this::updateLowerCaseToDatabaseName,
+ (remoteName, localName) ->
lowerCaseToDatabaseName.put(remoteName.toLowerCase(), remoteName),
Review Comment:
[P1] Apply database filters before publishing create events
The full loader applies `include_database_list`/`exclude_database_list`
before replacing this routing map, but this keyed callback publishes every HMS
`CREATE_DATABASE` event. `registerDatabaseFromEvent()` builds with
`checkExists=false` and never checks the current filters, so on a warm mode-2
catalog an event for an excluded database adds both the cached object and
(here) its lowercase route; differently-cased `getDbNullable()` calls can then
return a database the configured snapshot deliberately omits. This is distinct
from the existing reset/filter thread because no lifecycle change is
involved—the event and filter are both current. Please apply the current
include/exclude admission rule before the event update and add a warm mode-2
excluded-database event test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +162,282 @@ 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(false).stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ public List<String> refreshNames() {
+ throwIfInterrupted();
+ NamesLoad loadInProgress;
+ synchronized (namesMutationLock) {
+ loadInProgress = activeNamesLoad;
+ }
+ if (loadInProgress != null) {
+ try {
+ awaitNamesLoad(loadInProgress);
+ } catch (RuntimeException e) {
+ if (Thread.currentThread().isInterrupted()) {
+ throw e;
+ }
+ // This load started before the forced refresh and must not
decide its result.
+ }
+ }
+ throwIfInterrupted();
+ return
getNames(true).stream().map(Pair::value).collect(Collectors.toList());
Review Comment:
[P1] Exclude the load already awaited from the forced phase
The owner completes its future at line 304 but does not clear
`activeNamesLoad` until its `finally`. A waiter released here can therefore
enter `getNames(true)` during that window, see the same current-generation
owner, join its already-completed future again, and accept the
pre-forced-refresh snapshot without starting a new physical load. A warm mode-2
miss can consequently remain false when that old loader captured names before
the remote create; an ignored recoverable failure can likewise be rethrown on
the second join. Please make the forced phase exclude the exact load it already
awaited (or transition active ownership before completing it) and add a
deterministic completion-before-cleanup regression.
--
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]
