wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3655725512
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -205,65 +363,199 @@ public MetaCacheEntryStats stats() {
lastError.get());
}
- // Read the config dynamically so existing cache entries follow runtime
config updates.
- private boolean isManualMissLoadEnabled() {
- return Config.enable_external_meta_cache_manual_miss_load;
- }
-
// Execute slow miss loads outside Caffeine's sync load path and suppress
stale write-back after invalidation.
- private V getWithManualLoad(K key, Function<K, V> loadFunction) {
+ private V getWithManualLoad(K key, Function<K, V> loadFunction,
+ @Nullable BiPredicate<K, V> currentValueActionRequired,
+ @Nullable BiConsumer<K, V> currentValueAction) {
if (!effectiveEnabled) {
- // Bypass cache entirely when the entry is disabled so manual miss
load does not relax disable semantics.
- return loadAndTrack(key, loadFunction);
+ if (currentValueAction == null) {
+ // Preserve the ordinary disabled-entry path without adding
publication coordination.
+ return loadAndTrack(key, loadFunction);
+ }
+ // Bypass object publication when disabled, but still fence an
auxiliary-index update against invalidation.
+ long generation;
+ synchronized (publishLock(key)) {
+ generation = generationOf(key);
+ }
+ V loaded = loadAndTrack(key, loadFunction);
+ if (loaded == null) {
+ return loaded;
+ }
+ beforeCurrentValueActionForTest(key, loaded);
+ synchronized (publishLock(key)) {
+ if (generation == generationOf(key)
+ && currentValueActionRequired.test(key, loaded)) {
+ currentValueAction.accept(key, loaded);
+ }
+ }
+ return loaded;
}
V value = data.getIfPresent(key);
if (value != null) {
+ runCurrentValueActionIfPresent(
+ key, value, currentValueActionRequired,
currentValueAction);
return value;
}
+ // Keep the slow miss load under the per-key load lock so concurrent
misses for the same key
+ // are still deduplicated. publishLock(key) only protects the short
publication window.
synchronized (loadLock(key)) {
value = data.asMap().get(key);
if (value != null) {
+ runCurrentValueActionIfPresent(
+ key, value, currentValueActionRequired,
currentValueAction);
return value;
}
- long generation = invalidateGeneration.get();
- V loaded = loadAndTrack(key, loadFunction);
- if (generation != invalidateGeneration.get()) {
- return loaded;
+ long generation;
+ // Snapshot generation only after re-checking the cache under the
publication lock so
+ // public mutations cannot slip between the miss observation and
the captured version.
+ synchronized (publishLock(key)) {
+ value = data.asMap().get(key);
+ if (value != null) {
+ if (currentValueAction != null
+ && currentValueActionRequired.test(key, value)) {
+ currentValueAction.accept(key, value);
+ }
+ return value;
+ }
+ generation = generationOf(key);
}
-
- // Keep null results uncached so manual miss load matches
LoadingCache null-return behavior.
+ V loaded = loadAndTrack(key, loadFunction);
if (loaded == null) {
return null;
}
- // Leave a narrow hook for tests to pause exactly before the cache
put race window.
- beforeManualCachePutForTest(key, loaded);
- data.put(key, loaded);
- if (generation != invalidateGeneration.get()) {
- removeLoadedValue(key, loaded);
+ synchronized (publishLock(key)) {
+ if (generation != generationOf(key)) {
+ return loaded;
+ }
+ // Leave a narrow hook for tests to pause exactly before the
cache put race window.
+ beforeManualCachePutForTest(key, loaded);
+ putLoadedValueWithoutGenerationBump(key, loaded);
+ // Re-check after the put because
invalidateAll()/invalidateIf() may bump stripe generations
+ // outside publishLock while this thread is publishing a
loaded value.
+ if (generation != generationOf(key)) {
+ removeLoadedValueWithoutGenerationBump(key, loaded);
+ } else if (currentValueAction != null
+ && currentValueActionRequired.test(key, loaded)) {
+ currentValueAction.accept(key, loaded);
+ }
}
return loaded;
}
}
+ private void runCurrentValueActionIfPresent(K key, V value,
+ @Nullable BiPredicate<K, V> currentValueActionRequired,
+ @Nullable BiConsumer<K, V> currentValueAction) {
+ if (currentValueAction == null) {
+ return;
+ }
+ if (!currentValueActionRequired.test(key, value)) {
+ return;
+ }
+ beforeCurrentValueActionForTest(key, value);
+ synchronized (publishLock(key)) {
+ if (data.asMap().get(key) == value
+ && currentValueActionRequired.test(key, value)) {
+ currentValueAction.accept(key, value);
+ }
+ }
+ }
+
+ // Keep internal load write-back separate from public mutation so it does
not advance generation.
+ private void putLoadedValueWithoutGenerationBump(K key, V loaded) {
+ data.put(key, loaded);
+ }
+
// Remove only the value loaded by the current request and keep newer
replacements intact.
- private void removeLoadedValue(K key, V loaded) {
+ private void removeLoadedValueWithoutGenerationBump(K key, V loaded) {
data.asMap().computeIfPresent(key, (ignored, currentValue) ->
currentValue == loaded ? null : currentValue);
}
+ private CacheLoader<K, V> newCacheLoader() {
+ return new CacheLoader<K, V>() {
+ @Override
+ public V load(K key) {
+ return loadFromDefaultLoader(key);
+ }
+
+ @Override
+ public CompletableFuture<V> asyncReload(K key, V oldValue,
Executor executor) {
+ long generation = generationOf(key);
Review Comment:
Documented the intended consistency boundary in `74941ba59fb`; no locking
behavior is changed in this commit.
The described overlap is real, but the cache protocol intentionally
guarantees the narrower rule stated in the updated PR description: work
admitted under an older generation must not publish after a mutation. A refresh
admitted after the generation bump but before key removal is treated as
overlapping/post-bump work; Caffeine may repopulate the key, and that small
window is accepted under the external metadata cache eventual-consistency
contract.
The comments on `publishLocks`, loader construction, public mutation, and
`asyncReload()` now make this boundary explicit so the implementation is not
read as providing full refresh-vs-invalidation linearizability. We therefore
are not moving refresh admission under `publishLock` or adding the proposed
overlap test in this PR.
--
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]
