github-actions[bot] commented on code in PR #67417:
URL: https://github.com/apache/doris/pull/67417#discussion_r3920895240
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -74,24 +131,131 @@ 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() {
+ int maxRetries = 3;
+ for (int attempt = 0; attempt < maxRetries + 1; attempt++) {
+ 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;
+ }
+ if (attempt == maxRetries - 1) {
+ return value.names;
+ }
+ }
+ NamesCacheValue value = namesCache.getIfPresent("");
+ return value != null ? value.names : Lists.newArrayList();
+ }
+
+ private NamesCacheValue loadNames(boolean forceRefresh) {
+ long loadGeneration;
+ List<Pair<String, String>> incompleteNames;
+ synchronized (namesDedupLock) {
+ synchronized (namesMutationLock) {
+ NamesCacheValue cached = namesCache.getIfPresent("");
+ if (!forceRefresh && cached != null && cached.complete
+ && cached.generation == namesGeneration.get()) {
+ return cached;
+ }
+ loadGeneration = namesGeneration.get();
+ if (!forceRefresh && activeLoadGeneration.get() ==
loadGeneration) {
Review Comment:
[P1] Join the current generation's active load
If caller A is paused in the cold `namesCacheLoader.load()`, caller B
reaches this branch with the same generation and gets `null` on all four
immediate retries. Since A has not published yet, B then returns `[]` (or a
`complete=false` event overlay) from the terminal fallback. That makes
concurrent database/table listings empty and can make mode-2 lookups or
`isTableExist()` reject valid objects even though the load succeeds. Please
keep a per-generation in-flight result that same-generation callers join, while
still allowing a newer post-invalidation generation to load independently, and
add a two-cold-reader test. This is distinct from the existing mutation-churn
and obsolete-generation threads because it requires no mutation or generation
change.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -127,26 +291,40 @@ 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));
+ synchronized (namesMutationLock) {
+ long generation = namesGeneration.incrementAndGet();
+ NameMutation mutation = NameMutation.update(remoteName, localName);
+ NamesCacheValue current = namesCache.getIfPresent("");
+ if (current != null) {
+ List<Pair<String, String>> names =
Lists.newArrayList(current.names);
Review Comment:
[P2] Keep incremental events out of full-snapshot publication
For a warm complete namespace, every `updateCache()` now clones and scans
the entire names list, then `publishNames()` rebuilds the entire database/table
routing map before releasing `namesMutationLock`, which every `getNames()`
reader also needs. A batch of HMS create events therefore becomes O(events
times namespace size) allocation/work and stalls metadata readers; the base
path appended the event entry and updated the routing map incrementally. Please
keep event mutations in a keyed/O(1)-amortized structure and use the targeted
callback, reserving full snapshot publication for loader completion/reset, with
a large-namespace repeated-event test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -127,26 +291,40 @@ 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));
+ synchronized (namesMutationLock) {
+ long generation = namesGeneration.incrementAndGet();
+ NameMutation mutation = NameMutation.update(remoteName, localName);
+ NamesCacheValue current = namesCache.getIfPresent("");
+ if (current != null) {
+ List<Pair<String, String>> names =
Lists.newArrayList(current.names);
+ mutation.apply(names);
+ NamesCacheValue updated = new NamesCacheValue(generation,
names, current.complete);
+ namesCache.put("", updated);
+ publishNames(updated);
Review Comment:
[P1] Do not bulk-publish an incomplete names overlay
After a previously complete names entry expires, the separate case-routing
map still contains the old names. The first event update takes the `current ==
null` branch, caches a `complete=false` singleton, and correctly merges that
name into the retained map. A second update reaches this call with another
`complete=false` value, so the bulk callbacks replace the entire routing map
with only the two overlay entries and drop every older database/table mapping.
Replay/cache-only mode-2 lookups then miss those objects, and a normal catalog
lookup also misses them if enumeration fails. Please use the targeted
update/invalidation callbacks while `current.complete` is false and reserve
whole-map replacement for a validated complete load/reset; add an expiry plus
two-mutation test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java:
##########
@@ -180,6 +369,48 @@ public void addObjForTest(long id, String name, T db) {
* Should only be used after creating new database/table
*/
public void resetNames() {
- namesCache.invalidateAll();
+ synchronized (namesMutationLock) {
+ minimumLoadGeneration = namesGeneration.incrementAndGet();
+ namesCache.invalidateAll();
+ namesCacheUpdateAction.accept(Lists.newArrayList());
Review Comment:
[P1] Preserve replay routing on a names-only reset
`resetNames()` deliberately leaves `metaObjCache` intact, but this new
callback erases the mode-2 routing map that name-based replay needs to reach
those cached objects. On a follower, replaying a rename unregisters the old
table and calls this reset; if the next edit-log record refreshes an unrelated
cached table B by name, `getTableForReplay("B")` performs no remote listing,
cannot resolve B through the empty map, and returns before invalidating its
stale metadata. The base names-only reset retained that mapping. Please keep
routing for retained objects until they are invalidated, or make cache-only
replay resolve them without this map, and add a mode-2 rename-reset followed by
unrelated refresh replay test.
--
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]
