smengcl commented on code in PR #4567:
URL: https://github.com/apache/ozone/pull/4567#discussion_r1244618933
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -224,29 +225,33 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache>
get(String key,
* @param key snapshot table key
*/
public void release(String key) {
- ReferenceCounted<IOmMetadataReader, SnapshotCache> rcOmSnapshot =
- dbMap.get(key);
- Preconditions.checkNotNull(rcOmSnapshot,
- "Key '" + key + "' does not exist in cache");
-
- if (rcOmSnapshot.decrementRefCount() == 0L) {
- synchronized (pendingEvictionList) {
- // Eligible to be closed, add it to the list.
- pendingEvictionList.add(rcOmSnapshot);
- // The cache size might have already exceed the soft limit
- // Thus triggering cleanup() to check and evict if applicable
- cleanup();
+ dbMap.compute(key, (k, v) -> {
+ if (v == null) {
+ throw new IllegalArgumentException(
+ "Key '" + key + "' does not exist in cache");
}
- }
+
+ if (v.decrementRefCount() == 0L) {
+ synchronized (pendingEvictionList) {
+ // Eligible to be closed, add it to the list.
+ pendingEvictionList.add(v);
+ // The cache size might have already exceed the soft limit
+ // Thus triggering cleanup() to check and evict if applicable
+ cleanup();
+ }
+ }
+
+ return v;
Review Comment:
nvm. turns out it is a bad idea to update other mappings inside a
`map.compute()` anyways according to its javadoc. In this case it corrupts the
map size count, and it caught by my UT:
https://github.com/apache/ozone/actions/runs/5396424029/jobs/9800041580?pr=4567#step:6:2594
Repro snippet:
```java
import java.util.concurrent.ConcurrentHashMap;
class Untitled {
public static void main(String[] args) {
ConcurrentHashMap map = new ConcurrentHashMap<String, String>();
String k1 = "k1";
map.put(k1, "v1");
map.put("k2", "v2");
System.out.println("- Map dump 1");
map.forEach((k, v) -> {
System.out.format("k=%s, v=%s\n", k, v, map.size());
});
System.out.println();
System.out.println("- compute()");
map.compute(k1, (k, v) -> {
System.out.format("k=%s, v=%s, size=%d\n", k, v, map.size());
map.remove(k);
System.out.format("k=%s, v=%s, size=%d\n", k, v, map.size());
return map.get(k);
});
System.out.println();
// Map size becomes incorrect at this point
System.out.format("k=%s, v=%s, size=%d\n", k1, map.get(k1), map.size());
System.out.println("- Map dump 2");
map.forEach((k, v) -> {
System.out.format("k=%s, v=%s\n", k, v, map.size());
});
}
}
```
Output:
```
- Map dump 1
k=k1, v=v1
k=k2, v=v2
- compute()
k=k1, v=v1, size=2
k=k1, v=v1, size=1
k=k1, v=null, size=0
- Map dump 2
k=k2, v=v2
```
You can see map's `size=0` while it obviously still has `k2` entry in it.
I have moved `cleanup()` out of `compute()` and added additional syncing.
--
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]
