wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3637517398
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -141,36 +200,75 @@ public V getIfPresent(K key) {
return data.getIfPresent(key);
}
+ @Nullable
+ public V findIfPresent(Predicate<K> keyPredicate) {
+ if (!effectiveEnabled) {
+ return null;
+ }
+ // Replay-only fallback needs a cache-only scan over current hot keys
without triggering load-through.
+ for (java.util.Map.Entry<K, V> entry : data.asMap().entrySet()) {
+ if (keyPredicate.test(entry.getKey())) {
+ return entry.getValue();
+ }
+ }
+ return null;
+ }
+
public void put(K key, V value) {
+ // Public mutations participate in generation control so in-flight
loads cannot overwrite them later.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(value, "value can not be null");
if (!effectiveEnabled) {
return;
}
- data.put(key, value);
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ data.put(key, value);
+ }
+ }
+
+ public V compute(K key, BiFunction<K, V, V> remappingFunction) {
+ // Public compute must also advance the stripe generation before
mutating the cache state.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(remappingFunction, "remappingFunction can not
be null");
+ if (!effectiveEnabled) {
+ return null;
+ }
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ return data.asMap().compute(key, remappingFunction);
+ }
}
public void invalidateKey(K key) {
- invalidateGeneration.incrementAndGet();
- if (data.asMap().remove(key) != null) {
- invalidateCount.incrementAndGet();
+ Objects.requireNonNull(key, "key can not be null");
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ if (data.asMap().remove(key) != null) {
+ invalidateCount.incrementAndGet();
+ }
}
}
public void invalidateIf(Predicate<K> predicate) {
- invalidateGeneration.incrementAndGet();
- data.asMap().keySet().removeIf(key -> {
+ Objects.requireNonNull(predicate, "predicate can not be null");
+ // Cover in-flight manual loads whose keys are still outside the cache
map.
+ bumpAllGenerations();
Review Comment:
Thanks for pointing this out. This is a valid performance concern, but its
practical impact is limited: it requires a selective invalidation to overlap an
unrelated in-flight load, and the loaded result is still returned correctly;
only its cache publication is skipped, which may cause a later remote reload.
A safe fix is non-trivial because `invalidateIf()` must fence only matching
cached and in-flight keys while preserving the stale-load publication
guarantees. Doing that requires more targeted generation/in-flight tracking and
dedicated concurrency coverage, rather than simply removing the global bump.
To avoid expanding this late-stage correctness PR, we will leave this as a
follow-up and have added it to the existing external metadata cache performance
tracker,
[#65779](https://github.com/apache/doris/issues/65779#issuecomment-5057540787).
The follow-up will retain the global bump for `invalidateAll()` and cover the
A-invalidation/B-load case.
--
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]
