aswinshakil commented on code in PR #5968:
URL: https://github.com/apache/ozone/pull/5968#discussion_r1448141200
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -231,23 +216,20 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache>
get(String key,
* @param key snapshot table key
*/
public void release(String key) {
- ReferenceCounted<IOmMetadataReader, SnapshotCache>
- rcOmSnapshot = dbMap.get(key);
- if (rcOmSnapshot == null) {
- throw new IllegalArgumentException(
- "Key '" + key + "' does not exist in cache");
- }
-
- if (rcOmSnapshot.decrementRefCount() == 0L) {
- synchronized (pendingEvictionList) {
- // v is eligible to be evicted and closed
- pendingEvictionList.add(rcOmSnapshot);
+ Lock lock = getLock(key);
+ lock.lock();
+ try {
+ ReferenceCounted<IOmMetadataReader, SnapshotCache> rcOmSnapshot =
dbMap.get(key);
+ if (rcOmSnapshot == null) {
+ throw new IllegalArgumentException("Key '" + key + "' does not exist
in cache");
}
+ rcOmSnapshot.decrementRefCount();
+ // The cache size might have already exceeded the soft limit
+ // Thus triggering cleanup() to check and evict if applicable
+ cleanup();
Review Comment:
Though this is not used currently, We can move this as well.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -163,66 +150,64 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache>
get(String key)
* @param key snapshot table key
* @return an OmSnapshot instance, or null on error
*/
- public ReferenceCounted<IOmMetadataReader, SnapshotCache> get(String key,
- boolean skipActiveCheck) throws IOException {
- // Atomic operation to initialize the OmSnapshot instance (once) if the key
- // does not exist, and increment the reference count on the instance.
- ReferenceCounted<IOmMetadataReader, SnapshotCache> rcOmSnapshot =
- dbMap.compute(key, (k, v) -> {
- if (v == null) {
- LOG.info("Loading snapshot. Table key: {}", k);
- try {
- v = new ReferenceCounted<>(cacheLoader.load(k), false, this);
- } catch (OMException omEx) {
- // Return null if the snapshot is no longer active
- if (!omEx.getResult().equals(FILE_NOT_FOUND)) {
- throw new IllegalStateException(omEx);
+ public ReferenceCounted<IOmMetadataReader, SnapshotCache> get(String key,
boolean skipActiveCheck)
+ throws IOException {
+ Lock lock = getLock(key);
+ lock.lock();
+ ReferenceCounted<IOmMetadataReader, SnapshotCache> rcOmSnapshot;
+ try {
+ rcOmSnapshot =
+ dbMap.compute(key, (k, v) -> {
+ if (v == null) {
+ LOG.info("Loading snapshot. Table key: {}", k);
+ try {
+ v = new ReferenceCounted<>(cacheLoader.load(k), false, this);
+ } catch (OMException omEx) {
+ // Return null if the snapshot is no longer active
+ if (!omEx.getResult().equals(FILE_NOT_FOUND)) {
+ throw new IllegalStateException(omEx);
+ }
+ } catch (IOException ioEx) {
+ // Failed to load snapshot DB
+ throw new IllegalStateException(ioEx);
+ } catch (Exception ex) {
+ // Unexpected and unknown exception thrown from
CacheLoader#load
+ throw new IllegalStateException(ex);
}
- } catch (IOException ioEx) {
- // Failed to load snapshot DB
- throw new IllegalStateException(ioEx);
- } catch (Exception ex) {
- // Unexpected and unknown exception thrown from CacheLoader#load
- throw new IllegalStateException(ex);
}
- }
- return v;
- });
-
- if (rcOmSnapshot == null) {
- // The only exception that would fall through the loader logic above
- // is OMException with FILE_NOT_FOUND.
- throw new OMException("Snapshot table key '" + key + "' not found, "
- + "or the snapshot is no longer active",
- OMException.ResultCodes.FILE_NOT_FOUND);
- } else {
- // When RC OmSnapshot is successfully loaded
- rcOmSnapshot.incrementRefCount();
- }
-
- // If the snapshot is already loaded in cache, the check inside the loader
- // above is ignored. But we would still want to reject all get()s except
- // when called from SDT (and some) if the snapshot is not active anymore.
- if (!skipActiveCheck &&
- !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
- // Ref count was incremented. Need to decrement on exception here.
- rcOmSnapshot.decrementRefCount();
- throw new OMException("Unable to load snapshot. " +
- "Snapshot with table key '" + key + "' is no longer active",
- FILE_NOT_FOUND);
- }
+ if (v != null) {
+ // When RC OmSnapshot is successfully loaded
+ v.incrementRefCount();
+ }
+ return v;
+ });
+ if (rcOmSnapshot == null) {
+ // The only exception that would fall through the loader logic above
+ // is OMException with FILE_NOT_FOUND.
+ throw new OMException("Snapshot table key '" + key + "' not found, "
+ + "or the snapshot is no longer active",
+ OMException.ResultCodes.FILE_NOT_FOUND);
+ }
- synchronized (pendingEvictionList) {
- // Remove instance from clean up list when it exists.
- pendingEvictionList.remove(rcOmSnapshot);
+ // If the snapshot is already loaded in cache, the check inside the
loader
+ // above is ignored. But we would still want to reject all get()s except
+ // when called from SDT (and some) if the snapshot is not active anymore.
+ if (!skipActiveCheck &&
+ !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
+ // Ref count was incremented. Need to decrement on exception here.
+ rcOmSnapshot.decrementRefCount();
+ throw new OMException("Unable to load snapshot. " +
+ "Snapshot with table key '" + key + "' is no longer active",
+ FILE_NOT_FOUND);
+ }
+ } finally {
+ lock.unlock();
}
// Check if any entries can be cleaned up.
// At this point, cache size might temporarily exceed cacheSizeLimit
- // even if there are entries that can be evicted, which is fine since it
- // is a soft limit.
+ // even if there are entries that can be evicted, which is fine since it
is a soft limit.
cleanup();
Review Comment:
Now that this is moved outside of the lock I think it should be fine with
the deadlock case.
--
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]
