wenzhenghu commented on code in PR #64705:
URL: https://github.com/apache/doris/pull/64705#discussion_r3463911108
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -155,31 +188,96 @@ public MetaCacheEntryStats stats() {
cacheStats.hitCount(),
cacheStats.missCount(),
cacheStats.hitRate(),
- cacheStats.loadSuccessCount(),
- cacheStats.loadFailureCount(),
- cacheStats.totalLoadTime(),
- cacheStats.averageLoadPenalty(),
+ successCount,
+ failureCount,
+ totalLoadTime,
+ totalLoadCount == 0 ? 0D : (double) totalLoadTime /
totalLoadCount,
cacheStats.evictionCount(),
invalidateCount.get(),
lastLoadSuccessTimeMs.get(),
lastLoadFailureTimeMs.get(),
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) {
+ V value = data.getIfPresent(key);
+ if (value != null) {
+ return value;
+ }
+
+ synchronized (loadLock(key)) {
+ value = data.asMap().get(key);
+ if (value != null) {
+ return value;
+ }
+
+ long generation = invalidateGeneration.get();
+ V loaded = loadAndTrack(key, loadFunction);
+ if (generation != invalidateGeneration.get()) {
+ return loaded;
+ }
+
+ // Keep null results uncached so manual miss load matches
LoadingCache null-return behavior.
+ if (loaded == null) {
Review Comment:
Thanks, this is a valid issue and we will fix it.
After checking the old behavior more carefully, I think the right
characterization is:
- before the manual miss-load change, disabled entries were already not
perfectly strict from a cache-visibility perspective, because the underlying
Caffeine cache is still constructed with maximumSize(0), and direct
put/getIfPresent can temporarily observe a value before maintenance;
- however, the old LoadingCache.get(...) path still preserved the main
disabled-cache behavior for normal reads, because repeated get(...) calls would
reload instead of stably serving a cached value;
- the new manual miss-load path makes this materially worse, because it
explicitly does getIfPresent(...) + put(...), so a disabled entry can now serve
a loaded value on the normal get(...) path, which is a real semantics
regression.
So I agree this should be fixed in MetaCacheEntry itself.
The fix I plan to make is:
- when effectiveEnabled is false, the manual path will bypass cache entirely
and directly load-and-return;
- getIfPresent(...) will return null for disabled entries;
- put(...) will become a no-op for disabled entries.
This keeps the manual miss-load goal intact by avoiding Caffeine synchronous
miss loading, and it also restores the expected disabled-cache contract. I will
add a focused FE unit test for this case as well.
--
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]
