wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3541958269
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -142,35 +201,60 @@ public V getIfPresent(K key) {
}
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();
+ for (K key : data.asMap().keySet()) {
if (predicate.test(key)) {
- invalidateCount.incrementAndGet();
- return true;
+ invalidateKey(key);
}
- return false;
- });
+ }
}
public void invalidateAll() {
- invalidateGeneration.incrementAndGet();
- long size = data.estimatedSize();
- data.invalidateAll();
- invalidateCount.addAndGet(size);
+ // Cover in-flight manual loads whose keys are still outside the cache
map.
Review Comment:
Thank you for the suggestion here.
I looked at this again, and for now I would prefer to keep the current
behavior. My understanding is that the two bumps are serving slightly different
purposes: `bumpAllGenerations()` covers in-flight manual loads whose keys may
still be outside the cache map, while the per-key invalidation path keeps the
normal publication/removal flow consistent for already-published entries.
So although it looks like a double bump from the counter perspective, it is
currently part of the stale write-back protection design. Since the benefit of
changing it seems limited compared with the risk of subtly changing
invalidation semantics, I have kept it unchanged 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]
