smengcl commented on code in PR #5968:
URL: https://github.com/apache/ozone/pull/5968#discussion_r1449027840
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -28,64 +29,54 @@
import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Collections;
+import java.util.HashMap;
import java.util.Iterator;
-import java.util.LinkedHashSet;
import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_NOT_FOUND;
import static
org.apache.hadoop.ozone.om.helpers.SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE;
/**
* Thread-safe custom unbounded LRU cache to manage open snapshot DB instances.
*/
-public class SnapshotCache implements ReferenceCountedCallback {
+public class SnapshotCache {
static final Logger LOG = LoggerFactory.getLogger(SnapshotCache.class);
-
// Snapshot cache internal hash map.
// Key: DB snapshot table key
// Value: OmSnapshot instance, each holds a DB instance handle inside
// TODO: [SNAPSHOT] Consider wrapping SoftReference<> around
IOmMetadataReader
- private final ConcurrentHashMap<String,
- ReferenceCounted<IOmMetadataReader, SnapshotCache>> dbMap;
+ private final Map<String, ReferenceCounted<IOmMetadataReader,
SnapshotCache>> dbMap;
- // Linked hash set that holds OmSnapshot instances whose reference count
- // has reached zero. Those entries are eligible to be evicted and closed.
- // Sorted in last used order.
- // Least-recently-used entry located at the beginning.
- private final Set<
- ReferenceCounted<IOmMetadataReader, SnapshotCache>> pendingEvictionList;
private final OmSnapshotManager omSnapshotManager;
private final CacheLoader<String, OmSnapshot> cacheLoader;
// Soft-limit of the total number of snapshot DB instances allowed to be
// opened on the OM.
private final int cacheSizeLimit;
+ private final Striped<ReadWriteLock> striped;
Review Comment:
Adding a javadoc paragraph above this line to explain the intended usage of
this for other maintainers would be appreciated.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -28,64 +29,54 @@
import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Collections;
+import java.util.HashMap;
import java.util.Iterator;
-import java.util.LinkedHashSet;
import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_NOT_FOUND;
import static
org.apache.hadoop.ozone.om.helpers.SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE;
/**
* Thread-safe custom unbounded LRU cache to manage open snapshot DB instances.
*/
-public class SnapshotCache implements ReferenceCountedCallback {
+public class SnapshotCache {
static final Logger LOG = LoggerFactory.getLogger(SnapshotCache.class);
-
// Snapshot cache internal hash map.
// Key: DB snapshot table key
// Value: OmSnapshot instance, each holds a DB instance handle inside
// TODO: [SNAPSHOT] Consider wrapping SoftReference<> around
IOmMetadataReader
- private final ConcurrentHashMap<String,
- ReferenceCounted<IOmMetadataReader, SnapshotCache>> dbMap;
+ private final Map<String, ReferenceCounted<IOmMetadataReader,
SnapshotCache>> dbMap;
- // Linked hash set that holds OmSnapshot instances whose reference count
- // has reached zero. Those entries are eligible to be evicted and closed.
- // Sorted in last used order.
- // Least-recently-used entry located at the beginning.
- private final Set<
- ReferenceCounted<IOmMetadataReader, SnapshotCache>> pendingEvictionList;
private final OmSnapshotManager omSnapshotManager;
private final CacheLoader<String, OmSnapshot> cacheLoader;
// Soft-limit of the total number of snapshot DB instances allowed to be
// opened on the OM.
private final int cacheSizeLimit;
+ private final Striped<ReadWriteLock> striped;
public SnapshotCache(
OmSnapshotManager omSnapshotManager,
CacheLoader<String, OmSnapshot> cacheLoader,
int cacheSizeLimit) {
- this.dbMap = new ConcurrentHashMap<>();
- this.pendingEvictionList =
- Collections.synchronizedSet(new LinkedHashSet<>());
+ this.dbMap = new HashMap<>();
Review Comment:
Could thread visibility be an issue here with plain `HashMap` since `dbMap`
can be mutated by multiple threads and we are not use `synchronize` on this
particular map?
https://stackoverflow.com/a/5169133
We likely still need `ConcurrentHashMap` here.
--
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]
