jojochuang commented on code in PR #9869:
URL: https://github.com/apache/ozone/pull/9869#discussion_r2954780871
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -511,4 +513,134 @@ void testSnapshotOperationsNotBlockedDuringCompaction()
throws IOException, Inte
verify(store1, times(1)).compactTable("table2");
verify(store1, times(0)).compactTable("keyTable");
}
+
+ private static IOzoneManagerLock newAcquiringLock() {
+ IOzoneManagerLock acquiringLock = mock(IOzoneManagerLock.class);
+ when(acquiringLock.acquireReadLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseReadLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
+ when(acquiringLock.acquireResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
+ when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
+ return acquiringLock;
+ }
+
+ private OmSnapshot mockSnapshot(UUID snapshotId) {
+ final OmSnapshot omSnapshot = mock(OmSnapshot.class);
+ when(omSnapshot.getSnapshotTableKey()).thenReturn(snapshotId.toString());
+ when(omSnapshot.getSnapshotID()).thenReturn(snapshotId);
+
+ return omSnapshot;
+ }
+
+ @Test
+ @DisplayName("Stale eviction key (invalidate + late close) is cleaned up
without throwing")
+ void testStaleEvictionKeyDuringCleanup() throws IOException {
+ snapshotCache = new SnapshotCache(cacheLoader, CACHE_SIZE_LIMIT,
omMetrics, 0, true, newAcquiringLock());
+ final UUID snapshotId = UUID.randomUUID();
+
+ // Acquire a snapshot handle so it is ref-counted in the cache.
+ UncheckedAutoCloseableSupplier<OmSnapshot> handle =
snapshotCache.get(snapshotId);
+ assertEquals(1, snapshotCache.size());
+
+ // Invalidate removes the dbMap entry. The handle still exists and will
later hit refcount=0.
+ snapshotCache.invalidate(snapshotId);
+ assertEquals(0, snapshotCache.size());
+
+ // Late close triggers ReferenceCounted callback which can re-add
snapshotId to pendingEvictionQueue.
+ handle.close();
+ assertTrue(snapshotCache.getPendingEvictionQueue().contains(snapshotId));
+
+ // cleanup(true) is invoked by lock(); it should remove the stale key and
not throw.
+ assertDoesNotThrow(() -> {
+ try (UncheckedAutoCloseableSupplier<OMLockDetails> lockDetails =
snapshotCache.lock()) {
+ assertTrue(lockDetails.get().isLockAcquired());
+ }
+ });
+ assertFalse(snapshotCache.getPendingEvictionQueue().contains(snapshotId));
+ }
+
+ @Test
+ @DisplayName("Close failure keeps snapshot in eviction queue for retry")
+ void testCloseFailureRetriesSnapshot() throws Exception {
+
+ snapshotCache = new SnapshotCache(cacheLoader, CACHE_SIZE_LIMIT,
omMetrics, 0, true, newAcquiringLock());
+ final UUID snapshotId = UUID.randomUUID();
+
+ final AtomicBoolean failCloseOnce = new AtomicBoolean(true);
+ final OmSnapshot failingSnapshot = mockSnapshot(snapshotId);
+
+ OMMetadataManager metadataManager = mock(OMMetadataManager.class);
+ DBStore store = mock(DBStore.class);
+ when(failingSnapshot.getMetadataManager()).thenReturn(metadataManager);
+ when(metadataManager.getStore()).thenReturn(store);
+ when(store.listTables()).thenReturn(new ArrayList<>());
+
+ doAnswer(invocation -> {
+ if (failCloseOnce.getAndSet(false)) {
+ throw new IOException("close failed");
+ }
+ return null;
+ }).when(failingSnapshot).close();
+
+ when(cacheLoader.load(eq(snapshotId))).thenReturn(failingSnapshot);
+
+ // Load + close handle so refcount transitions to 0 and snapshotId is
queued for eviction.
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> ignored =
snapshotCache.get(snapshotId)) {
+ assertEquals(1, snapshotCache.size());
+ assertEquals(1, omMetrics.getNumSnapshotCacheSize());
+ }
+ assertEquals(0L,
snapshotCache.getDbMap().get(snapshotId).getTotalRefCount());
+ assertTrue(snapshotCache.getPendingEvictionQueue().contains(snapshotId));
+
+ // First cleanup attempt fails to close; entry should remain in dbMap and
key should stay queued for retry.
+ assertThrows(IllegalStateException.class, () -> snapshotCache.lock());
Review Comment:
+1
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -323,8 +329,13 @@ private UncheckedAutoCloseableSupplier<OMLockDetails>
lock(Supplier<OMLockDetail
AtomicReference<OMLockDetails> lockDetails = new
AtomicReference<>(emptyLockFunction.get());
if (lockDetails.get().isLockAcquired()) {
- if (!cleanupFunction.get()) {
+ try {
+ if (!cleanupFunction.get()) {
+ throw new IllegalStateException("Failed to acquire lock as cleanup
did not drain the cache.");
+ }
+ } catch (Throwable t) {
lockDetails.set(emptyUnlockFunction.get());
Review Comment:
+1
--
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]
