lhotari commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r1347804842
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +74,13 @@ public RangeCache(Weighter<Value> weighter,
TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ MutableBoolean flag = new MutableBoolean();
+ entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ flag.setValue(true);
+ return value;
+ });
+ return flag.booleanValue();
Review Comment:
@mattisonchao This doesn't seem to be a thread safe change here.
`computeIfAbsent` doesn't lock the key when ConcurrentSkipListMap is used.
You can see this in the source code of `ConcurrentMap`:
```
default V computeIfAbsent(K key,
Function<? super K, ? extends V> mappingFunction) {
Objects.requireNonNull(mappingFunction);
V oldValue, newValue;
return ((oldValue = get(key)) == null
&& (newValue = mappingFunction.apply(key)) != null
&& (oldValue = putIfAbsent(key, newValue)) == null)
? newValue
: oldValue;
}
```
On the other hand, ConcurrentHashMap.computeIfAbsent does lock the key and
atomically call the mappingFunction. More details in a tweet where some
observations by @michaeljmarshall were shared:
https://twitter.com/spyced/status/1709677859764154819 .
--
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]
