github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r3910439004
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -81,7 +90,32 @@ public MetaCache(String name,
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ List<Pair<String, String>> names = namesCache.getIfPresent("");
Review Comment:
[P1] Fence automatic refresh publication too
In Caffeine 2.9.3 this `getIfPresent("")` is not passive: an eligible hit
runs `afterRead -> refreshIfNeeded`. If that async reload starts before
`invalidateAll()`/`resetNames()`, its completion sees the mapping as absent and
inserts the loaded value without consulting `namesGeneration` or
`namesMutationLock`. Both production callers enable `refreshAfterWrite`, while
the new test disables it, so `REFRESH CATALOG` can still be undone by a
pre-refresh reload. Please route automatic refresh through the same
generation-aware publication path (or disable Caffeine-owned refresh and
implement a fenced reload), and cover the production refresh configuration in
the test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -81,7 +90,32 @@ public MetaCache(String name,
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ List<Pair<String, String>> names = namesCache.getIfPresent("");
+ if (names == null) {
+ synchronized (namesLoadLock) {
+ names = namesCache.getIfPresent("");
+ if (names == null) {
+ long loadGeneration = namesGeneration.get();
+ names = loadNames();
Review Comment:
[P1] Fence the loaders' side effects as well
Rejecting the returned list here does not reject everything the load
published. The catalog loader clears/fills `lowerCaseToDatabaseName`, and the
table loader clears/fills `lowerCaseToTableName`, before this generation check.
A pre-refresh load can therefore resume after the reset, repopulate the
new/cleared case-routing map with its old snapshot, and still be rejected only
from `namesCache`; case-insensitive lookups then trust that stale hit without
reloading. Please make the loaders side-effect free and publish the derived map
with the list under the same generation fence, or otherwise discard/rebuild
those maps when the load loses the generation check.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -81,7 +90,32 @@ public MetaCache(String name,
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ List<Pair<String, String>> names = namesCache.getIfPresent("");
+ if (names == null) {
+ synchronized (namesLoadLock) {
+ names = namesCache.getIfPresent("");
+ if (names == null) {
+ long loadGeneration = namesGeneration.get();
+ names = loadNames();
+ synchronized (namesMutationLock) {
+ if (loadGeneration == namesGeneration.get()) {
+ namesCache.put("", names);
+ }
+ }
+ }
+ }
+ }
+ return names.stream().map(Pair::value).collect(Collectors.toList());
+ }
+
+ private List<Pair<String, String>> loadNames() {
+ try {
+ return Objects.requireNonNull(namesCacheLoader.load(""));
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
Review Comment:
[P2] Restore the interrupt before wrapping it
The replaced Caffeine 2.9.3 loader adapter catches `InterruptedException`
separately, calls `Thread.currentThread().interrupt()`, and then throws
`CompletionException`. Here it falls into the generic `Exception` catch, so an
interruptible `CacheLoader` loses its cancellation signal after this change.
Please mirror the dedicated interrupted-exception handling and add a focused
assertion for the flag.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -81,7 +90,32 @@ public MetaCache(String name,
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ List<Pair<String, String>> names = namesCache.getIfPresent("");
+ if (names == null) {
+ synchronized (namesLoadLock) {
+ names = namesCache.getIfPresent("");
+ if (names == null) {
+ long loadGeneration = namesGeneration.get();
Review Comment:
[P1] Make cold loads atomic with incremental mutations
This generation sample is not ordered with the miss check or the incremental
mutation. If `updateCache`/`invalidate` runs after the sample, it installs a
singleton/empty entry, the full load is rejected, and later reads hit that
incomplete entry instead of retrying. In the reverse window, a mutation can
increment/publish between line 96 and this sample; the loader then observes the
new generation and later overwrites that explicit mutation. The old same-key
Caffeine load/compute ordering serialized both cases. Please make the sample
plus cache recheck atomic with `namesMutationLock` and ensure an incremental
mutation cannot turn an incomplete cold load into an authoritative
singleton/empty entry; add paused-load tests for both update and per-name
invalidate.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/MetaCacheTest.java:
##########
@@ -158,6 +159,47 @@ public void testInvalidateAll() {
Assert.assertFalse(metaCache.getMetaObj("local2", 2L).isPresent());
}
+ @Test
+ public void testInvalidateNamesRejectsInFlightLoad() throws Exception {
+ CountDownLatch firstLoadStarted = new CountDownLatch(1);
+ CountDownLatch releaseFirstLoad = new CountDownLatch(1);
+ AtomicInteger loadCount = new AtomicInteger();
+ CacheLoader<String, List<Pair<String, String>>> namesCacheLoader = key
-> {
+ int currentLoad = loadCount.incrementAndGet();
+ if (currentLoad == 1) {
+ firstLoadStarted.countDown();
+ Assert.assertTrue(releaseFirstLoad.await(3, TimeUnit.SECONDS));
+ }
+ return Lists.newArrayList(Pair.of("remote-" + currentLoad,
"local-" + currentLoad));
+ };
+ ExecutorService refreshExecutor = Executors.newSingleThreadExecutor();
+ ExecutorService callers = Executors.newFixedThreadPool(2);
+ MetaCache<String> cache = new MetaCache<>(
+ "databaseCache",
+ refreshExecutor,
+ OptionalLong.empty(),
+ OptionalLong.empty(),
+ 10,
+ namesCacheLoader,
+ key -> Optional.of(key),
+ (key, value, cause) -> { });
+
+ Future<List<String>> firstLoad = callers.submit(cache::listNames);
+ Assert.assertTrue(firstLoadStarted.await(3, TimeUnit.SECONDS));
Review Comment:
[P2] Cover the initial wait with executor cleanup
The first submitted load and this await occur before the `try/finally`. If
startup times out or the test thread is interrupted here, `releaseFirstLoad` is
never released and neither executor is shut down, leaving a non-daemon
fixed-pool worker blocked in the loader. Please begin the cleanup scope before
submitting/awaiting the first load (and ideally await termination) so failure
paths cannot strand the test JVM.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -81,7 +90,32 @@ public MetaCache(String name,
}
public List<String> listNames() {
- return
Objects.requireNonNull(namesCache.get("")).stream().map(Pair::value).collect(Collectors.toList());
+ List<Pair<String, String>> names = namesCache.getIfPresent("");
+ if (names == null) {
+ synchronized (namesLoadLock) {
+ names = namesCache.getIfPresent("");
+ if (names == null) {
+ long loadGeneration = namesGeneration.get();
+ names = loadNames();
+ synchronized (namesMutationLock) {
+ if (loadGeneration == namesGeneration.get()) {
+ namesCache.put("", names);
+ }
+ }
+ }
+ }
+ }
+ return names.stream().map(Pair::value).collect(Collectors.toList());
Review Comment:
[P1] Preserve the list/remote-name lookup contract
On a generation mismatch this returns the completed list while deliberately
leaving `namesCache` empty. In mapped/lowercased catalogs, `buildDbForInit` and
`buildTableForInit` enumerate through `listNames()` and then immediately call
`getRemoteName()`, which does `requireNonNull(namesCache.getIfPresent(""))`. A
concurrent invalidate/reset can therefore turn the newly supported "return but
do not publish" case into an NPE that escapes the object-cache loader even when
the requested name was in the returned list. Please have remote-name resolution
share the generation-aware acquisition/result, or pass the loaded pairs through
instead of assuming every returned list was cached; add a mapped-name test for
this sequence.
--
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]
