smengcl commented on code in PR #10774:
URL: https://github.com/apache/ozone/pull/10774#discussion_r3642898248
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/defrag/TestSnapshotDefragService.java:
##########
@@ -964,6 +965,69 @@ public void testCheckAndDefragSnapshotFailure(boolean
previousSnapshotExists) th
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCheckpointCleanupOnDefragFailure(boolean
previousSnapshotExists) throws IOException {
+ SnapshotInfo snapshotInfo = createMockSnapshotInfo(UUID.randomUUID(),
"vol1", "bucket1", "snap2");
+ SnapshotInfo previousSnapshotInfo;
+ if (previousSnapshotExists) {
+ previousSnapshotInfo = createMockSnapshotInfo(UUID.randomUUID(), "vol1",
"bucket1", "snap1");
+
snapshotInfo.setPathPreviousSnapshotId(previousSnapshotInfo.getSnapshotId());
+ } else {
+ previousSnapshotInfo = null;
+ }
+
+ SnapshotChainManager chainManager = mock(SnapshotChainManager.class);
+ try (MockedStatic<SnapshotUtils> mockedStatic =
Mockito.mockStatic(SnapshotUtils.class)) {
+ mockedStatic.when(() -> SnapshotUtils.getSnapshotInfo(eq(ozoneManager),
eq(chainManager),
+ eq(snapshotInfo.getSnapshotId()))).thenReturn(snapshotInfo);
+ if (previousSnapshotExists) {
+ mockedStatic.when(() ->
SnapshotUtils.getSnapshotInfo(eq(ozoneManager), eq(chainManager),
+
eq(previousSnapshotInfo.getSnapshotId()))).thenReturn(previousSnapshotInfo);
+ }
+
+ SnapshotDefragService spyDefragService = Mockito.spy(defragService);
+ doReturn(Pair.of(true,
10)).when(spyDefragService).needsDefragmentation(eq(snapshotInfo));
+
+ @SuppressWarnings("resource") // Mock object, no actual resource
management needed
+ OmMetadataManagerImpl checkpointMetadataManager =
mock(OmMetadataManagerImpl.class);
+ File checkpointPath = tempDir.resolve("checkpoint_" +
System.nanoTime()).toAbsolutePath().toFile();
+ // Create actual checkpoint directory to verify cleanup
+ assertTrue(checkpointPath.mkdirs(), "Failed to create checkpoint
directory for test");
+ assertTrue(checkpointPath.exists(), "Checkpoint directory should exist
before defragmentation");
+
+ DBStore checkpointDBStore = mock(DBStore.class);
+ when(checkpointMetadataManager.getStore()).thenReturn(checkpointDBStore);
+ when(checkpointDBStore.getDbLocation()).thenReturn(checkpointPath);
+ doNothing().when(checkpointMetadataManager).close();
+
doReturn(checkpointMetadataManager).when(spyDefragService).createCheckpoint(any(),
any());
+
+ TablePrefixInfo prefixInfo = new TablePrefixInfo(Collections.emptyMap());
+ when(metadataManager.getTableBucketPrefix(anyString(),
anyString())).thenReturn(prefixInfo);
+
+ // Make the defrag operation throw IOException to simulate failure
+ IOException defragException = new IOException("Defrag failed");
+ if (previousSnapshotExists) {
+
Mockito.doThrow(defragException).when(spyDefragService).performIncrementalDefragmentation(
+ any(), any(), anyInt(), any(), any(), any());
Review Comment:
This no longer compiles against current `master`. HDDS-15860 removed the
snapshot-version argument from `performIncrementalDefragmentation()`, so the
merged method takes five arguments while this stub passes six. I reproduced
`testCompile` failing at this call. Please rebase the patch and remove
`anyInt()`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -723,7 +730,24 @@ boolean checkAndDefragSnapshot(SnapshotChainManager
chainManager, UUID snapshotI
}
} finally {
if (checkpointMetadataManager != null) {
- checkpointMetadataManager.close();
+ try {
+ checkpointMetadataManager.close();
+ } catch (IOException closeException) {
+ LOG.error("Failed to close checkpoint metadata manager for snapshot:
{} (ID: {})",
+ snapshotInfo.getTableKey(), snapshotInfo.getSnapshotId(),
closeException);
+ }
+ }
+ if (!defragSuccessful && checkpointLocation.toFile().exists()) {
+ try {
+ deleteDirectory(checkpointLocation);
+ LOG.info("Cleaned up failed checkpoint directory for snapshot: {}
(ID: {})",
+ snapshotInfo.getTableKey(), snapshotInfo.getSnapshotId());
+ } catch (IOException cleanupException) {
+ LOG.error("Failed to delete checkpoint directory {} for snapshot: {}
(ID: {}). " +
+ "Disk space may not be freed. Manual cleanup may be required.",
+ checkpointLocation, snapshotInfo.getTableKey(),
snapshotInfo.getSnapshotId(), cleanupException);
+ snapshotMetrics.incNumSnapshotDefragFails();
Review Comment:
This can count one failed defragmentation twice: if cleanup fails while the
original `IOException` is propagating, the counter is incremented here, then
`triggerSnapshotDefragOnce()` increments the same counter again. Since this
metric counts failed defragmentation operations, please leave the increment to
the outer catch or use a separate cleanup-failure counter.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -709,7 +710,13 @@ boolean checkAndDefragSnapshot(SnapshotChainManager
chainManager, UUID snapshotI
checkpointMetadataManager = null;
// Switch the snapshot DB location to the new version.
previousVersion = atomicSwitchSnapshotDB(snapshotId,
checkpointLocation);
- omSnapshotManager.deleteSnapshotCheckpointDirectories(snapshotId,
previousVersion);
+ try {
+ omSnapshotManager.deleteSnapshotCheckpointDirectories(snapshotId,
previousVersion);
+ } catch (IOException deleteException) {
Review Comment:
Please preserve a retry path when deletion of the old checkpoint directories
fails. `atomicSwitchSnapshotDB()` has already committed the new version, so
subsequent scans may return `needsDefrag=false`. Catching the exception here
then continues to the success metrics, with no guaranteed retry of this
deletion; the old directories can remain until a later purge. Could cleanup be
retried independently of whether the snapshot still needs defragmentation?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -661,6 +661,7 @@ boolean checkAndDefragSnapshot(SnapshotChainManager
chainManager, UUID snapshotI
OmMetadataManagerImpl checkpointMetadataManager =
createCheckpoint(checkpointSnapshotInfo,
COLUMN_FAMILIES_TO_TRACK_IN_SNAPSHOT);
Path checkpointLocation =
checkpointMetadataManager.getStore().getDbLocation().toPath();
+ boolean defragSuccessful = false;
Review Comment:
Could we also cover failures inside `createCheckpoint()`? It creates the
checkpoint directory before opening, truncating, and reopening it, but this
`try/finally` is entered only after `createCheckpoint()` returns. An exception
during those steps still leaves a checkpoint directory under `tmp_defrag`, so
repeated failures can reproduce the accumulation this patch is intended to fix.
Please clean up once the `DBCheckpoint` exists and add failure coverage for
this path.
--
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]
