adoroszlai commented on code in PR #11841:
URL: https://github.com/apache/ozone/pull/11841#discussion_r4434440908
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -368,19 +373,118 @@ public void init() throws RocksDBException, IOException,
ExecutionException {
SnapshotCache snapshotCache = new SnapshotCache(mockCacheLoader(), 10,
omMetrics, 0, true,
new OmReadOnlyLock());
- when(omSnapshotManager.getActiveSnapshot(anyString(), anyString(),
anyString()))
- .thenAnswer(invocationOnMock -> {
- SnapshotInfo snapInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
invocationOnMock.getArgument(0),
- invocationOnMock.getArgument(1),
invocationOnMock.getArgument(2));
- return snapshotCache.get(snapInfo.getSnapshotId());
- });
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "ozoneManager",
ozoneManager);
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "snapshotCache",
snapshotCache);
+ Mockito.doCallRealMethod().when(omSnapshotManager)
+ .getActiveSnapshot(anyString(), anyString(), anyString(),
any(UUID.class));
when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager);
snapshotDiffManager = new SnapshotDiffManager(db, ozoneManager,
snapDiffJobTable, snapDiffReportTable, snapDiffPurgedJobTable,
columnFamilyOptions, codecRegistry);
when(omSnapshotManager.getDiffCleanupServiceInterval()).thenReturn(0L);
}
+ @ParameterizedTest
+ @CsvSource({"0, false, false", "1, false, false", "2, false, false", "3,
false, false",
+ "3, true, false", "0, false, true", "1, false, true", "2, false, true",
"3, false, true", "3, true, true"})
Review Comment:
nit: please use `@MethodSource`
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -658,6 +658,16 @@ public UncheckedAutoCloseableSupplier<OmSnapshot>
getActiveSnapshot(
return getSnapshot(volumeName, bucketName, snapshotName, false);
}
+ /** Opens an active snapshot only if its name still identifies the expected
UUID. */
+ public UncheckedAutoCloseableSupplier<OmSnapshot> getActiveSnapshot(
+ String volumeName, String bucketName, String snapshotName, UUID
expectedSnapshotId) throws IOException {
+ Objects.requireNonNull(expectedSnapshotId, "expectedSnapshotId");
+ if (snapshotName == null || snapshotName.isEmpty()) {
+ throw new OMException(INVALID_KEY_NAME);
Review Comment:
Should we throw `OMException` for `expectedSnapshotId == null` instead of
unchecked one?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -368,19 +373,118 @@ public void init() throws RocksDBException, IOException,
ExecutionException {
SnapshotCache snapshotCache = new SnapshotCache(mockCacheLoader(), 10,
omMetrics, 0, true,
new OmReadOnlyLock());
- when(omSnapshotManager.getActiveSnapshot(anyString(), anyString(),
anyString()))
- .thenAnswer(invocationOnMock -> {
- SnapshotInfo snapInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
invocationOnMock.getArgument(0),
- invocationOnMock.getArgument(1),
invocationOnMock.getArgument(2));
- return snapshotCache.get(snapInfo.getSnapshotId());
- });
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "ozoneManager",
ozoneManager);
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "snapshotCache",
snapshotCache);
+ Mockito.doCallRealMethod().when(omSnapshotManager)
+ .getActiveSnapshot(anyString(), anyString(), anyString(),
any(UUID.class));
when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager);
snapshotDiffManager = new SnapshotDiffManager(db, ozoneManager,
snapDiffJobTable, snapDiffReportTable, snapDiffPurgedJobTable,
columnFamilyOptions, codecRegistry);
when(omSnapshotManager.getDiffCleanupServiceInterval()).thenReturn(0L);
}
+ @ParameterizedTest
+ @CsvSource({"0, false, false", "1, false, false", "2, false, false", "3,
false, false",
+ "3, true, false", "0, false, true", "1, false, true", "2, false, true",
"3, false, true", "3, true, true"})
+ public void testSnapshotDiffLocks(int failOnOpen, boolean sameSnapshot,
boolean optimized) throws Exception {
+ SnapshotInfo from = snapshotInfo;
+ SnapshotInfo to = sameSnapshot ? from : snapshotInfoList.get(0);
+ OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration());
+ when(omMetadataManager.getLock()).thenReturn(lock);
+ try (SnapshotCache cache = new SnapshotCache(mockCacheLoader(), 10,
ozoneManager.getMetrics(), 0, true, lock)) {
+ SnapshotDiffManager manager =
spy(newSnapshotDiffManagerWithOptimizedEnabled(optimized));
+ doReturn(failOnOpen != 0,
false).when(manager).areDiffJobAndSnapshotsActive(
+ VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName());
+ AtomicInteger opens = new AtomicInteger();
+ OmSnapshotManager omSnapshotManager =
ozoneManager.getOmSnapshotManager();
+ doAnswer(invocation -> {
+ // Both stripes must be held before even the first snapshot is opened.
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
from.getSnapshotId().toString()) > 0);
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
to.getSnapshotId().toString()) > 0);
+ if (opens.incrementAndGet() == failOnOpen) {
+ throw new IOException("Snapshot load failed");
+ }
+ String name = invocation.getArgument(2);
+ return cache.get(name.equals(from.getName()) ? from.getSnapshotId() :
to.getSnapshotId());
+ }).when(omSnapshotManager).getActiveSnapshot(anyString(), anyString(),
anyString(), any(UUID.class));
+ String jobKey = generateSnapDiffJobKey.apply(from, to);
+ String jobId = UUID.randomUUID().toString();
+ manager.getSnapDiffJobTable().put(jobKey, new
SnapshotDiffJob(System.currentTimeMillis(), jobId,
+ IN_PROGRESS, VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName(),
false, false, 0, null, 0.0, ""));
+ if (failOnOpen == 1 || failOnOpen == 2) {
+ RuntimeException failure = assertThrows(RuntimeException.class, () ->
manager.generateSnapshotDiffReport(
+ jobKey, jobId, VOLUME_NAME, BUCKET_NAME, from.getName(),
to.getName(), true, false, false));
+ assertEquals("Snapshot load failed", failure.getCause().getMessage());
+ assertEquals(FAILED,
manager.getSnapDiffJobTable().get(jobKey).getStatus());
+ } else {
+ manager.generateSnapshotDiffReport(jobKey, jobId, VOLUME_NAME,
BUCKET_NAME,
+ from.getName(), to.getName(), true, false, false);
+ }
+ assertEquals(failOnOpen == 0 ? 0 : Math.min(failOnOpen, 2), opens.get());
+ assertEquals(0, lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
from.getSnapshotId().toString()));
+ assertEquals(0, lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
to.getSnapshotId().toString()));
+ assertEquals(0, cache.totalRefCount(from.getSnapshotId()));
+ assertEquals(0, cache.totalRefCount(to.getSnapshotId()));
+ }
+ }
+
+ @ParameterizedTest
+ @org.junit.jupiter.params.provider.ValueSource(ints = {1, 2})
+ public void testSnapshotDiffLocksAfterSnapshotNameReuse(int replaceOnOpen)
throws Exception {
+ SnapshotInfo from = snapshotInfo;
+ SnapshotInfo to = snapshotInfoList.get(0);
+ OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration());
+ when(omMetadataManager.getLock()).thenReturn(lock);
+ try (SnapshotCache cache = new SnapshotCache(mockCacheLoader(), 10,
ozoneManager.getMetrics(), 0, true, lock)) {
+ SnapshotDiffManager manager = spy(snapshotDiffManager);
+ doReturn(true, false).when(manager).areDiffJobAndSnapshotsActive(
+ VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName());
+ Set<UUID> openedIds = new LinkedHashSet<>();
+ Set<UUID> replacementIds = new LinkedHashSet<>();
+ AtomicInteger opens = new AtomicInteger();
+ OmSnapshotManager omSnapshotManager =
ozoneManager.getOmSnapshotManager();
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager,
"snapshotCache", cache);
+ doAnswer(invocation -> {
+ if (opens.incrementAndGet() == replaceOnOpen) {
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
from.getSnapshotId().toString()) > 0);
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
to.getSnapshotId().toString()) > 0);
+ UUID replacementId = LongStream.range(0, 1024).mapToObj(id -> new
UUID(0, id))
+ .filter(id -> lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
id.toString()) == 0)
+ .findFirst().orElseThrow(() -> new AssertionError("No unheld
snapshot DB stripe found"));
+ // Simulate A being renamed away and C taking A's old name after A
and B were prelocked.
+ SnapshotInfo original = replaceOnOpen == 1 ? from : to;
+ SnapshotInfo replacement =
original.toBuilder().setSnapshotId(replacementId).build();
+ replacementIds.add(replacementId);
+
when(snapshotInfoTable.get(original.getTableKey())).thenReturn(replacement);
+ }
+ UUID id = invocation.getArgument(3);
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK, id.toString()) > 0);
+ Object handle = invocation.callRealMethod();
+ openedIds.add(id);
+ return handle;
+ }).when(omSnapshotManager).getActiveSnapshot(anyString(), anyString(),
anyString(), any(UUID.class));
+ String jobKey = generateSnapDiffJobKey.apply(from, to);
+ String jobId = UUID.randomUUID().toString();
+ manager.getSnapDiffJobTable().put(jobKey, new
SnapshotDiffJob(System.currentTimeMillis(), jobId,
+ IN_PROGRESS, VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName(),
false, false, 0, null, 0.0, ""));
+
+ RuntimeException failure = assertThrows(RuntimeException.class, () ->
manager.generateSnapshotDiffReport(
+ jobKey, jobId, VOLUME_NAME, BUCKET_NAME, from.getName(),
to.getName(), true, false, false));
+ assertTrue(failure.getCause().getMessage().contains("Snapshot name
mapping changed"));
Review Comment:
nit: use `assertThat(exception).cause().hasMessageContaining(...)` (see
HDDS-9951)
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1182,12 +1189,18 @@ void generateSnapshotDiffReport(final String jobKey,
// throw this exception.
throw new RuntimeException(exception);
} finally {
- // Decrement ref counts
- if (rcFromSnapshot != null) {
- rcFromSnapshot.close();
- }
- if (rcToSnapshot != null) {
- rcToSnapshot.close();
+ try {
+ // Decrement ref counts
+ if (rcFromSnapshot != null) {
+ rcFromSnapshot.close();
+ }
+ if (rcToSnapshot != null) {
+ rcToSnapshot.close();
+ }
Review Comment:
Use `IOUtils.close(rcFromSnapshot, rcToSnapshot)` to ensure exception in
first does not prevent closing the other one.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -368,19 +373,118 @@ public void init() throws RocksDBException, IOException,
ExecutionException {
SnapshotCache snapshotCache = new SnapshotCache(mockCacheLoader(), 10,
omMetrics, 0, true,
new OmReadOnlyLock());
- when(omSnapshotManager.getActiveSnapshot(anyString(), anyString(),
anyString()))
- .thenAnswer(invocationOnMock -> {
- SnapshotInfo snapInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
invocationOnMock.getArgument(0),
- invocationOnMock.getArgument(1),
invocationOnMock.getArgument(2));
- return snapshotCache.get(snapInfo.getSnapshotId());
- });
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "ozoneManager",
ozoneManager);
+ HddsWhiteboxTestUtils.setInternalState(omSnapshotManager, "snapshotCache",
snapshotCache);
+ Mockito.doCallRealMethod().when(omSnapshotManager)
+ .getActiveSnapshot(anyString(), anyString(), anyString(),
any(UUID.class));
when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager);
snapshotDiffManager = new SnapshotDiffManager(db, ozoneManager,
snapDiffJobTable, snapDiffReportTable, snapDiffPurgedJobTable,
columnFamilyOptions, codecRegistry);
when(omSnapshotManager.getDiffCleanupServiceInterval()).thenReturn(0L);
}
+ @ParameterizedTest
+ @CsvSource({"0, false, false", "1, false, false", "2, false, false", "3,
false, false",
+ "3, true, false", "0, false, true", "1, false, true", "2, false, true",
"3, false, true", "3, true, true"})
+ public void testSnapshotDiffLocks(int failOnOpen, boolean sameSnapshot,
boolean optimized) throws Exception {
+ SnapshotInfo from = snapshotInfo;
+ SnapshotInfo to = sameSnapshot ? from : snapshotInfoList.get(0);
+ OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration());
+ when(omMetadataManager.getLock()).thenReturn(lock);
+ try (SnapshotCache cache = new SnapshotCache(mockCacheLoader(), 10,
ozoneManager.getMetrics(), 0, true, lock)) {
+ SnapshotDiffManager manager =
spy(newSnapshotDiffManagerWithOptimizedEnabled(optimized));
+ doReturn(failOnOpen != 0,
false).when(manager).areDiffJobAndSnapshotsActive(
+ VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName());
+ AtomicInteger opens = new AtomicInteger();
+ OmSnapshotManager omSnapshotManager =
ozoneManager.getOmSnapshotManager();
+ doAnswer(invocation -> {
+ // Both stripes must be held before even the first snapshot is opened.
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
from.getSnapshotId().toString()) > 0);
+ assertTrue(lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
to.getSnapshotId().toString()) > 0);
+ if (opens.incrementAndGet() == failOnOpen) {
+ throw new IOException("Snapshot load failed");
+ }
+ String name = invocation.getArgument(2);
+ return cache.get(name.equals(from.getName()) ? from.getSnapshotId() :
to.getSnapshotId());
+ }).when(omSnapshotManager).getActiveSnapshot(anyString(), anyString(),
anyString(), any(UUID.class));
+ String jobKey = generateSnapDiffJobKey.apply(from, to);
+ String jobId = UUID.randomUUID().toString();
+ manager.getSnapDiffJobTable().put(jobKey, new
SnapshotDiffJob(System.currentTimeMillis(), jobId,
+ IN_PROGRESS, VOLUME_NAME, BUCKET_NAME, from.getName(), to.getName(),
false, false, 0, null, 0.0, ""));
+ if (failOnOpen == 1 || failOnOpen == 2) {
+ RuntimeException failure = assertThrows(RuntimeException.class, () ->
manager.generateSnapshotDiffReport(
+ jobKey, jobId, VOLUME_NAME, BUCKET_NAME, from.getName(),
to.getName(), true, false, false));
+ assertEquals("Snapshot load failed", failure.getCause().getMessage());
+ assertEquals(FAILED,
manager.getSnapDiffJobTable().get(jobKey).getStatus());
+ } else {
+ manager.generateSnapshotDiffReport(jobKey, jobId, VOLUME_NAME,
BUCKET_NAME,
+ from.getName(), to.getName(), true, false, false);
+ }
+ assertEquals(failOnOpen == 0 ? 0 : Math.min(failOnOpen, 2), opens.get());
+ assertEquals(0, lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
from.getSnapshotId().toString()));
+ assertEquals(0, lock.getReadHoldCount(SNAPSHOT_DB_LOCK,
to.getSnapshotId().toString()));
+ assertEquals(0, cache.totalRefCount(from.getSnapshotId()));
+ assertEquals(0, cache.totalRefCount(to.getSnapshotId()));
+ }
+ }
+
+ @ParameterizedTest
+ @org.junit.jupiter.params.provider.ValueSource(ints = {1, 2})
Review Comment:
nit: add import
--
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]
