hemantk-12 commented on code in PR #5968:
URL: https://github.com/apache/ozone/pull/5968#discussion_r1449275708
##########
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:
You made a good point. I never heard before that `HashMap` could lead to
infinite loop if multiple thread are updating it.
But using `ConcurrentHashMap` with locks could lead to the same problem as
#5751.
There is another approach to just use the `ConcurrentHashMap`. I created
draft PR: #5986 according to that. My only concern is if it will throw
`ConcurrentModificationException` while iterating over the map when another
thread is updating or adding entries to it. I read couple of blogs and seems
like it should not be a problem if I go with compute function
https://stackoverflow.com/questions/37127285/iterate-over-concurrenthashmap-while-deleting-entries
and https://www.digitalocean.com/community/tutorials/concurrenthashmap-in-java
Let me know what you think about this approach.
--
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]
