This is an automated email from the ASF dual-hosted git repository.
weichiu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new f0f2a0a2959 HDDS-13904. Exception handling correctly should release
snapshot read lock (#9271)
f0f2a0a2959 is described below
commit f0f2a0a295952dfd42d8755b5c5368ca1440e956
Author: Swaminathan Balachandran <[email protected]>
AuthorDate: Wed Nov 12 20:30:26 2025 -0500
HDDS-13904. Exception handling correctly should release snapshot read lock
(#9271)
---
.../hadoop/ozone/om/snapshot/SnapshotCache.java | 102 +++++++++++----------
.../ozone/om/snapshot/TestSnapshotCache.java | 70 +++++++-------
2 files changed, 91 insertions(+), 81 deletions(-)
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
index ff0b04b0541..c0580cdd16e 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
@@ -17,7 +17,6 @@
package org.apache.hadoop.ozone.om.snapshot;
-import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_NOT_FOUND;
import static org.apache.hadoop.ozone.om.lock.FlatResource.SNAPSHOT_DB_LOCK;
import static
org.apache.ozone.rocksdiff.RocksDBCheckpointDiffer.COLUMN_FAMILIES_TO_TRACK_IN_DAG;
@@ -211,59 +210,62 @@ public UncheckedAutoCloseableSupplier<OmSnapshot>
get(UUID key) throws IOExcepti
throw new OMException("Unable to acquire readlock on snapshot db with
key " + key,
OMException.ResultCodes.INTERNAL_ERROR);
}
- // Atomic operation to initialize the OmSnapshot instance (once) if the key
- // does not exist, and increment the reference count on the instance.
- ReferenceCounted<OmSnapshot> rcOmSnapshot =
- dbMap.compute(key, (k, v) -> {
- if (v == null) {
- LOG.info("Loading SnapshotId: '{}'", k);
- try {
- v = new ReferenceCounted<>(cacheLoader.load(key), 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);
+ try {
+ // Atomic operation to initialize the OmSnapshot instance (once) if the
key
+ // does not exist, and increment the reference count on the instance.
+ ReferenceCounted<OmSnapshot> rcOmSnapshot = dbMap.compute(key, (k, v) ->
{
+ if (v == null) {
+ LOG.info("Loading SnapshotId: '{}'", k);
+ try {
+ v = new ReferenceCounted<>(cacheLoader.load(key), false, this);
+ } catch (OMException omEx) {
+ // Return null if the snapshot is no longer active
+ if
(!omEx.getResult().equals(OMException.ResultCodes.FILE_NOT_FOUND)) {
+ throw new IllegalStateException(omEx);
}
- omMetrics.incNumSnapshotCacheSize();
+ } 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);
}
- 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.
- lock.releaseReadLock(SNAPSHOT_DB_LOCK, key.toString());
- throw new OMException("SnapshotId: '" + key + "' not found, or the
snapshot is no longer active.",
- OMException.ResultCodes.FILE_NOT_FOUND);
- }
- return new UncheckedAutoCloseableSupplier<OmSnapshot>() {
- private final AtomicReference<Boolean> closed = new
AtomicReference<>(false);
- @Override
- public OmSnapshot get() {
- return rcOmSnapshot.get();
+ omMetrics.incNumSnapshotCacheSize();
+ }
+ if (v != null) {
+ // When RC OmSnapshot is successfully loaded
+ v.incrementRefCount();
+ }
+ return v;
+ });
+ if (rcOmSnapshot == null) {
+ throw new OMException("SnapshotId: '" + key + "' not found, or the
snapshot is no longer active.",
+ OMException.ResultCodes.FILE_NOT_FOUND);
}
- @Override
- public void close() {
- closed.updateAndGet(alreadyClosed -> {
- if (!alreadyClosed) {
- rcOmSnapshot.decrementRefCount();
- lock.releaseReadLock(SNAPSHOT_DB_LOCK, key.toString());
- }
- return true;
- });
- }
- };
+ return new UncheckedAutoCloseableSupplier<OmSnapshot>() {
+ private final AtomicReference<Boolean> closed = new
AtomicReference<>(false);
+ @Override
+ public OmSnapshot get() {
+ return rcOmSnapshot.get();
+ }
+
+ @Override
+ public void close() {
+ closed.updateAndGet(alreadyClosed -> {
+ if (!alreadyClosed) {
+ rcOmSnapshot.decrementRefCount();
+ lock.releaseReadLock(SNAPSHOT_DB_LOCK, key.toString());
+ }
+ return true;
+ });
+ }
+ };
+ } catch (Throwable e) {
+ // Release the read lock irrespective of the exception thrown.
+ lock.releaseReadLock(SNAPSHOT_DB_LOCK, key.toString());
+ throw e;
+ }
}
/**
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
index e3de9653f1f..42368e6bf4b 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java
@@ -81,45 +81,42 @@ class TestSnapshotCache {
@BeforeAll
static void beforeAll() throws Exception {
cacheLoader = mock(CacheLoader.class);
- // Create a difference mock OmSnapshot instance each time load() is called
- when(cacheLoader.load(any())).thenAnswer(
- (Answer<OmSnapshot>) invocation -> {
- final OmSnapshot omSnapshot = mock(OmSnapshot.class);
- // Mock the snapshotTable return value for the lookup inside
release()
- final UUID snapshotID = (UUID) invocation.getArguments()[0];
-
when(omSnapshot.getSnapshotTableKey()).thenReturn(snapshotID.toString());
- when(omSnapshot.getSnapshotID()).thenReturn(snapshotID);
-
- OMMetadataManager metadataManager = mock(OMMetadataManager.class);
- org.apache.hadoop.hdds.utils.db.DBStore store =
mock(org.apache.hadoop.hdds.utils.db.DBStore.class);
- when(omSnapshot.getMetadataManager()).thenReturn(metadataManager);
- when(metadataManager.getStore()).thenReturn(store);
-
- Table<?, ?> table1 = mock(Table.class);
- Table<?, ?> table2 = mock(Table.class);
- Table<?, ?> keyTable = mock(Table.class);
- when(table1.getName()).thenReturn("table1");
- when(table2.getName()).thenReturn("table2");
- when(keyTable.getName()).thenReturn("keyTable"); // This is in
COLUMN_FAMILIES_TO_TRACK_IN_DAG
- final List<Table<?, ?>> tables = new ArrayList<>();
- tables.add(table1);
- tables.add(table2);
- tables.add(keyTable);
- when(store.listTables()).thenReturn(tables);
-
- return omSnapshot;
- }
- );
-
// Set SnapshotCache log level. Set to DEBUG for verbose output
GenericTestUtils.setLogLevel(SnapshotCache.class, Level.DEBUG);
lock = spy(new OmReadOnlyLock());
}
@BeforeEach
- void setUp() {
+ void setUp() throws Exception {
// Reset cache for each test case
omMetrics = OMMetrics.create();
+ // Create a difference mock OmSnapshot instance each time load() is called
+ doAnswer((Answer<OmSnapshot>) invocation -> {
+ final OmSnapshot omSnapshot = mock(OmSnapshot.class);
+ // Mock the snapshotTable return value for the lookup inside release()
+ final UUID snapshotID = (UUID) invocation.getArguments()[0];
+ when(omSnapshot.getSnapshotTableKey()).thenReturn(snapshotID.toString());
+ when(omSnapshot.getSnapshotID()).thenReturn(snapshotID);
+
+ OMMetadataManager metadataManager = mock(OMMetadataManager.class);
+ org.apache.hadoop.hdds.utils.db.DBStore store =
mock(org.apache.hadoop.hdds.utils.db.DBStore.class);
+ when(omSnapshot.getMetadataManager()).thenReturn(metadataManager);
+ when(metadataManager.getStore()).thenReturn(store);
+
+ Table<?, ?> table1 = mock(Table.class);
+ Table<?, ?> table2 = mock(Table.class);
+ Table<?, ?> keyTable = mock(Table.class);
+ when(table1.getName()).thenReturn("table1");
+ when(table2.getName()).thenReturn("table2");
+ when(keyTable.getName()).thenReturn("keyTable"); // This is in
COLUMN_FAMILIES_TO_TRACK_IN_DAG
+ final List<Table<?, ?>> tables = new ArrayList<>();
+ tables.add(table1);
+ tables.add(table2);
+ tables.add(keyTable);
+ when(store.listTables()).thenReturn(tables);
+
+ return omSnapshot;
+ }).when(cacheLoader).load(any(UUID.class));
snapshotCache = new SnapshotCache(cacheLoader, CACHE_SIZE_LIMIT,
omMetrics, 50, true, lock);
}
@@ -154,6 +151,17 @@ public void testGetFailsOnReadLock() throws IOException {
assertEquals(1, snapshotCache.size());
}
+ @Test
+ @DisplayName("Tests get() releases readLock when load() fails")
+ public void testGetReleasesReadLockOnLoadFailure() throws Exception {
+ clearInvocations(lock);
+ final UUID dbKey = UUID.randomUUID();
+ when(cacheLoader.load(eq(dbKey))).thenThrow(new Exception("Dummy exception
thrown"));
+ assertThrows(IllegalStateException.class, () -> snapshotCache.get(dbKey));
+ verify(lock, times(1)).acquireReadLock(eq(SNAPSHOT_DB_LOCK),
eq(dbKey.toString()));
+ verify(lock, times(1)).releaseReadLock(eq(SNAPSHOT_DB_LOCK),
eq(dbKey.toString()));
+ }
+
@ParameterizedTest
@ValueSource(ints = {0, 1, 5, 10})
@DisplayName("Tests get() holds a read lock")
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
