github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r4004842703
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +162,291 @@ 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);
Review Comment:
[P1] Let the forced miss bypass a hung names load
A mode-2 database/table miss calls `refreshNames()` synchronously, but this
unbounded pre-wait can sit forever behind a current-generation automatic
refresh that is stuck in connector I/O. The request never reaches
`getNames(true)`, even though only one of `MAX_PHYSICAL_NAMES_LOADS`' two slots
is occupied; before this change the miss path enumerated directly rather than
joining Caffeine's background reload. The new test releases the old load before
expecting the forced load, so it does not cover this liveness case. Please
retire/advance the pre-existing owner (or bound this wait and admit an
independent forced load) and test that the foreground lookup completes while
the old refresh remains blocked.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1268,11 +1317,15 @@ private String getLocalDatabaseName(String dbName,
boolean isReplay) {
// Mode 2: Case-insensitive comparison
finalName = lowerCaseToDatabaseName.get(dbName.toLowerCase());
if (finalName == null && !isReplay) {
- // Refresh database list and try again
try {
- getFilteredDatabaseNames();
+ metaCache.refreshNames();
finalName =
lowerCaseToDatabaseName.get(dbName.toLowerCase());
} catch (Exception e) {
+ if (e instanceof java.util.concurrent.CompletionException
Review Comment:
[P2] Do not inherit another load owner's interrupt
A caller can observe no active load in `refreshNames()`' pre-check, then
join a load another thread starts before it reaches `getNames(true)`. If only
that owner is interrupted, the shared future still delivers
`CompletionException(InterruptedException)` to this joiner, whose interrupt
flag remains clear; this branch then marks the unrelated query thread
interrupted and aborts it as though it had been cancelled. The earlier pre-wait
branch correctly distinguishes this with the current thread's flag. Please only
treat this as caller cancellation when that flag is set (otherwise
propagate/retry the shared load failure) and add a two-caller race test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -126,49 +481,96 @@ public Optional<T> getMetaObjById(long id) {
}
public void updateCache(String remoteName, String localName, T obj, long
id) {
- metaObjCache.put(localName, Optional.of(obj));
- namesCache.asMap().compute("", (k, v) -> {
- if (v == null) {
- return Lists.newArrayList(Pair.of(remoteName, localName));
- } else {
- v.add(Pair.of(remoteName, localName));
- return v;
+ updateCache(remoteName, localName, obj, id,
namesLoadEpochSupplier.getAsLong());
+ }
+
+ public boolean updateCache(String remoteName, String localName, T obj,
long id, long expectedEpoch) {
+ // Object loaders already use this monitor and can re-enter
listNames(). Keep the same lock
+ // order here, and never enter Caffeine's same-key mutation while
holding namesMutationLock.
+ synchronized (metaObjCache) {
Review Comment:
[P1] Keep unrelated events out of the object-loader monitor
A mode-0 lookup for database A can hold this cache-wide monitor while
`buildDbForInit()` sees a warm names snapshot that lacks A and blocks in its
direct `getFilteredDatabaseNames()` fallback. An HMS create/drop for unrelated
database B now waits here before publishing anything; because
`MetastoreEventsProcessor` processes events and catalogs sequentially, that one
query can stop all later events indefinitely. Before this change, B's
different-key Caffeine mutation and the already-warm names-key mutation did not
wait for A's direct enumeration. Please use per-key/striped coordination to
preserve same-key atomicity without coupling unrelated entries, and cover a
blocked database fallback alongside an event for another key.
--
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]
