You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sadanand48 (via GitHub)" <gi...@apache.org> on 2025/10/09 13:58:20 UTC
[PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
sadanand48 opened a new pull request, #9131:
URL: https://github.com/apache/ozone/pull/9131
## What changes were proposed in this pull request?
Snapshot Paths to be re read from om checkpoint db inside lock again from the checkpoint DB's metadataManager instance.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13772
## How was this patch tested?
unit test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9131:
URL: https://github.com/apache/ozone/pull/9131#issuecomment-3477992199
> > I see that we don't have a unit test for this class. Please add a unit test for this class which ensure we are taking a lock on all the background services and a double buffer wait happens
>
> We do have a unit test that verifies this and 2 tests that verify bootstrap lock co-ordinationi.e testBootstrapLockCoordination and testBootstrapLockBlocksMultipleServices
>
> https://github.com/apache/ozone/blob/833e955bdc237e4651142016b047fe376f4fc6dc/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java#L498
>
> I have updated the patch to remove the purge snapshot condition as you suggested.
Should we also write a test for OmDBCheckpointServletInodeBasedXfer is actually using OMDbCheckpointServlet.Lock and not something else? Here we are creating a new instance of the lock
https://github.com/apache/ozone/blob/833e955bdc237e4651142016b047fe376f4fc6dc/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java#L488
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2452614962
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -403,6 +416,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
+ Set<Path> snapshotPaths = new HashSet<>();
Review Comment:
maybe an assertion here to ensure the lock is held for consistency.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2481297570
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
@sadanand48 Let us purge the snapshot just before we are trying to take a checkpoint which means that we should purge the snapshot in this block
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #9131:
URL: https://github.com/apache/ozone/pull/9131#issuecomment-3477511995
> I see that we don't have a unit test for this class. Please add a unit test for this class which ensure we are taking a lock on all the background services and a double buffer wait happens
We do have a unit test that verifies this and 2 tests that verify bootstrap lock co-ordinationi.e testBootstrapLockCoordination and testBootstrapLockBlocksMultipleServices
https://github.com/apache/ozone/blob/833e955bdc237e4651142016b047fe376f4fc6dc/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java#L498
I have updated the patch to remove the purge snapshot condition as you suggested.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2482012266
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -228,12 +236,21 @@ public void write(int b) throws IOException {
.thenReturn(lock);
doCallRealMethod().when(omDbCheckpointServletMock).getCheckpoint(any(), anyBoolean());
assertNull(doCallRealMethod().when(omDbCheckpointServletMock).getBootstrapTempData());
- doCallRealMethod().when(omDbCheckpointServletMock).getSnapshotDirs(any());
doCallRealMethod().when(omDbCheckpointServletMock).
processMetadataSnapshotRequest(any(), any(), anyBoolean(), anyBoolean());
doCallRealMethod().when(omDbCheckpointServletMock).writeDbDataToStream(any(), any(), any(), any());
doCallRealMethod().when(omDbCheckpointServletMock).getCompactionLogDir();
doCallRealMethod().when(omDbCheckpointServletMock).getSstBackupDir();
+ doCallRealMethod().when(omDbCheckpointServletMock)
+ .transferSnapshotData(anySet(), any(), anySet(), any(), any(), anyMap());
+ doCallRealMethod().when(omDbCheckpointServletMock).createAndPrepareCheckpoint(anyBoolean());
+ doCallRealMethod().when(omDbCheckpointServletMock).getSnapshotDirsFromDB(any());
+ if (useNoOpBootstrapLock) {
+ // Override the lock to be a no-op so purgeSnapshot can work inside the callback
+ BootstrapStateHandler.Lock noOpLock = new DBCheckpointServlet.Lock();
+ when(omDbCheckpointServletMock.getBootstrapStateLock())
Review Comment:
We don't need a noop lock. We should pause the double buffer thread and delete the snapshot just before acquiring the lock.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2453934918
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -403,6 +416,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
+ Set<Path> snapshotPaths = new HashSet<>();
Review Comment:
there are no changes made to the checkpoint DB once a checkpoint has been taken so locks are not needed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465068381
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427383471
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +446,53 @@ public void testWriteDBToArchive(boolean expectOnlySstFiles) throws Exception {
}
}
+
+ /**
+ * SCENARIO:
+ * 1. Initially: S1->S2->S3 snapshots exist, snapshotPaths = {S1, S2, S3}
+ * 2. S3 gets purged (deleted from live OM metadata)
+ * 3. Checkpoint is created (locked point-in-time state without S3)
+ * 4. Problem: Old code uses stale snapshotPaths {S1, S2, S3} from step 1
+ * 5. Solution: Re-read snapshotPaths from checkpoint = {S1, S2} (no S3)
+ * This test verifies that checkpoint metadata manager (post-fix) excludes
+ * purged snapshots, while live metadata manager (pre-fix) would include them.
+ */
+ @Test
+ public void testSnapshotPathsReReadFromCheckpointAfterPurge() throws Exception {
Review Comment:
I'm not sure this test is valid. It seems to only invoke mock method rather than production code. Meaning: this test code would always be true regardless of the fix.
At very least it should call OmMetadataManagerImpl.createCheckpointMetadataManager() to get a check point.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427996251
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +446,53 @@ public void testWriteDBToArchive(boolean expectOnlySstFiles) throws Exception {
}
}
+
+ /**
+ * SCENARIO:
+ * 1. Initially: S1->S2->S3 snapshots exist, snapshotPaths = {S1, S2, S3}
+ * 2. S3 gets purged (deleted from live OM metadata)
+ * 3. Checkpoint is created (locked point-in-time state without S3)
+ * 4. Problem: Old code uses stale snapshotPaths {S1, S2, S3} from step 1
+ * 5. Solution: Re-read snapshotPaths from checkpoint = {S1, S2} (no S3)
+ * This test verifies that checkpoint metadata manager (post-fix) excludes
+ * purged snapshots, while live metadata manager (pre-fix) would include them.
+ */
+ @Test
+ public void testSnapshotPathsReReadFromCheckpointAfterPurge() throws Exception {
Review Comment:
purge sounded like a good test. anyway I will improve this test
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi merged PR #9131:
URL: https://github.com/apache/ozone/pull/9131
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2483765419
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #9131:
URL: https://github.com/apache/ozone/pull/9131#issuecomment-3479374599
@swamirishi When merging PRs, please remove co-author information if it's the same person with different email address.
```
Author: Sadanand Shenoy <sa...@...>
Date: Sun Nov 2 19:49:07 2025 +0530
HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. (#9131)
Co-authored-by: Sadanand Shenoy <sa...@...>
```
Also, please set fix version when resolving Jira issue after PR merge.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427988628
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
Review Comment:
we have already taken the active DB checkpoint before this stage and a new snapshot create at this stage would mean that the active DB checkpoint would not have a new entry in its' snapshotInfoTable.
we would miss the snaphsot but it will be created on the follower as part of raft log replay later
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465306244
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +410,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
Review Comment:
table iterator is thread safe. We can get rid of getSnapshotDirs function and just iterate through the snapshot info table of the metadataManager instance
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2481514499
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
Can we do the bootstrap on the follower OM? Bootstrap lock there won't mean anything when this runs on the follower. To get into this condition maybe we should mock bootstrap lock to do a noop here.
Also I think we mainly need to get the list of snapshots again mainly for handling snapshots on creates.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2452591709
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -258,6 +260,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
don't we need to acquire SNAPSHOT_LOCK here?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2464992873
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
@sadanand48 https://github.com/apache/ozone/commit/c6c4279fa129485049ce35a27ffde10d5e01d990#diff-d4e4dbf0e95db46eded21d40936d1b18cd738dc19fa10ec45dc88211b4b498e3
it does
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2481514499
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
Can we do the bootstrap on the follower OM? Bootstrap lock there won't mean anything when this runs on the follower. To get into this condition maybe we should mock bootstrap lock to do a noop here.
Actually what we should do is during BootstrapLock we should pause the double buffer thread delete the snapshot and acquire the bootstrap lock. Now just before the checkpoint we should unpause the double buffer thread and let the snapshot purge get flushed this would do it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2483812877
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +599,176 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2, S3
+ * 2. Purge S2
+ * 3. Checkpoint is created
+ * 4. S3 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S1, S3 data as
+ * checkpoint snapshotInfoTable has S1 S3
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshots = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshots::add);
+ assertEquals(2, snapshots.size(), "Should have 2 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshots.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ purgeSnapshot(volumeName, bucketName, snapshot2);
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+ // custom lock because the original lock waits for double buffer flush.
+ BootstrapStateHandler.Lock customLock = new BootstrapStateHandler.Lock() {
+ private final List<BootstrapStateHandler.Lock> serviceLocks;
Review Comment:
If that is the case I don't think purge snapshot race condition is even possible. Then we should just deal with the create snapshot race condition
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465062057
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +410,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
Review Comment:
1. getSnapshotDirs reads from the snapshotChainManager before the checkpoint has been taken.
2. getSnapshotDirsFromDB reads directly from the table from the checkpoint.
If I use method 2 throughout, would also need to handle locking aspect as we are reading from active DB which is prone to updates. We can leverage the in-memory chain manager that is already present when possible .
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2483814672
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java:
##########
@@ -83,6 +83,7 @@ public final class OzoneManagerDoubleBuffer {
private final Daemon daemon;
/** Is the {@link #daemon} running? */
private final AtomicBoolean isRunning = new AtomicBoolean(false);
+ private final AtomicBoolean isPaused = new AtomicBoolean(false);
Review Comment:
Why can't we use pauseDeamon() and resume() again?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465352568
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +404,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
+ Set<Path> snapshotPaths = new HashSet<>();
+ try (TableIterator<String, ? extends Table.KeyValue<String, SnapshotInfo>> iter =
+ omMetadataManager.getSnapshotInfoTable().iterator()) {
+ while (iter.hasNext()) {
+ Table.KeyValue<String, SnapshotInfo> kv = iter.next();
+ SnapshotInfo snapshotInfo = kv.getValue();
+ String snapshotDir = OmSnapshotManager.getSnapshotPath(getConf(),
Review Comment:
We don't need this wait.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2481374116
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
the checkpoint is taken after the bootstrap lock is acquired so once bootstrap lock is aquired the purge won't be succesful in this block as SDS needs to acquire this lock to do the purge. We need to do it outside the lock itself
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2453932489
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -258,6 +260,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
we are reading it from the checkpoint and already have the bootstrap lock held so it's not needed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465011435
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +410,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
Review Comment:
why have 2 functions?
Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) &
Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465306244
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +410,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
Review Comment:
table iterator is thread safe
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465959097
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
+ String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+ String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+
+ setupCluster();
+ om.getKeyManager().getSnapshotSstFilteringService().pause();
+
+ // Create test data and snapshots
+ OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client, volumeName, bucketName);
+
+ // Create key before first snapshot
+ TestDataUtil.createKey(bucket, "key1",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data1".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot1");
+
+ // Create key before second snapshot
+ TestDataUtil.createKey(bucket, "key2",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data2".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot2");
+ TestDataUtil.createKey(bucket, "key3",
+ ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, ReplicationFactor.ONE),
+ "data3".getBytes(StandardCharsets.UTF_8));
+ client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
+ om.getMetadataManager().getStore().flushDB();
+
+ // At this point: Live OM has snapshots S1, S2
+ List<OzoneSnapshot> snapshotsBeforePurge = new ArrayList<>();
+ client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+ .forEachRemaining(snapshotsBeforePurge::add);
+ assertEquals(3, snapshotsBeforePurge.size(), "Should have 3 snapshots initially");
+ OzoneSnapshot snapshot2 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot2"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+ OzoneSnapshot snapshot3 = snapshotsBeforePurge.stream()
+ .filter(snap -> snap.getName().equals("snapshot3"))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("snapshot2 not found"));
+
+ // Setup servlet mocks for checkpoint processing
+ setupMocks();
+ when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+
+ // Create a checkpoint that captures current state (S1, S2)
+ DBStore dbStore = om.getMetadataManager().getStore();
+ DBStore spyDbStore = spy(dbStore);
+ AtomicReference<DBCheckpoint> capturedCheckpoint = new AtomicReference<>();
+
+ when(spyDbStore.getCheckpoint(true)).thenAnswer(invocation -> {
+ DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+ doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
+ capturedCheckpoint.set(checkpoint);
+ return checkpoint;
+ });
Review Comment:
```suggestion
when(spyDbStore.getCheckpoint(eq(true))).thenAnswer(invocation -> {
client.getObjectStore().deleteSnapshot(volumeName, bucketName, "snapshot2");
client.getObjectStore().createSnapshot(volumeName, bucketName, "snapshot3");
// wait for snapshot2 to get purged and snapshot3 to get created.
DBCheckpoint checkpoint = spy(Mockito.callRealMethod());
doNothing().when(checkpoint).cleanupCheckpoint(); // Don't cleanup for verification
capturedCheckpoint.set(checkpoint);
return checkpoint;
});
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2483826655
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java:
##########
@@ -83,6 +83,7 @@ public final class OzoneManagerDoubleBuffer {
private final Daemon daemon;
/** Is the {@link #daemon} running? */
private final AtomicBoolean isRunning = new AtomicBoolean(false);
+ private final AtomicBoolean isPaused = new AtomicBoolean(false);
Review Comment:
Just saw the code we cannot use that
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2447784647
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +446,53 @@ public void testWriteDBToArchive(boolean expectOnlySstFiles) throws Exception {
}
}
+
+ /**
+ * SCENARIO:
+ * 1. Initially: S1->S2->S3 snapshots exist, snapshotPaths = {S1, S2, S3}
+ * 2. S3 gets purged (deleted from live OM metadata)
+ * 3. Checkpoint is created (locked point-in-time state without S3)
+ * 4. Problem: Old code uses stale snapshotPaths {S1, S2, S3} from step 1
+ * 5. Solution: Re-read snapshotPaths from checkpoint = {S1, S2} (no S3)
+ * This test verifies that checkpoint metadata manager (post-fix) excludes
+ * purged snapshots, while live metadata manager (pre-fix) would include them.
+ */
+ @Test
+ public void testSnapshotPathsReReadFromCheckpointAfterPurge() throws Exception {
Review Comment:
Done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427536133
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
```suggestion
try {
```
```suggestion
try (OmMetadataManager checkpointMetadataManager = OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint)) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
Review Comment:
Change getSnapshotDirs implementation to read the snapshotInfo table directly instead of building SnapshotChainManager which might be unoptimal since we build bucket chains unnecessarily
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +446,53 @@ public void testWriteDBToArchive(boolean expectOnlySstFiles) throws Exception {
}
}
+
+ /**
+ * SCENARIO:
+ * 1. Initially: S1->S2->S3 snapshots exist, snapshotPaths = {S1, S2, S3}
+ * 2. S3 gets purged (deleted from live OM metadata)
+ * 3. Checkpoint is created (locked point-in-time state without S3)
+ * 4. Problem: Old code uses stale snapshotPaths {S1, S2, S3} from step 1
+ * 5. Solution: Re-read snapshotPaths from checkpoint = {S1, S2} (no S3)
+ * This test verifies that checkpoint metadata manager (post-fix) excludes
+ * purged snapshots, while live metadata manager (pre-fix) would include them.
+ */
+ @Test
+ public void testSnapshotPathsReReadFromCheckpointAfterPurge() throws Exception {
Review Comment:
Why only purge this can also affect snapshot creates
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2426801889
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
+ } finally {
+ if (checkpointMetadataManager != null) {
+ checkpointMetadataManager.stop();
+ }
Review Comment:
Consider using try-with-resources pattern instead of manual resource management. If OmMetadataManagerImpl implements AutoCloseable, this would ensure proper cleanup even if an exception occurs during getSnapshotDirs().
```suggestion
try (OmMetadataManagerImpl checkpointMetadataManager =
OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint)) {
snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -284,7 +295,7 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
* @param hardLinkFileMap Map of hardlink file paths to their unique identifiers for deduplication.
* @throws IOException if an I/O error occurs during processing.
*/
- private void transferSnapshotData(Set<String> sstFilesToExclude, Path tmpdir, Set<Path> snapshotPaths,
+ void transferSnapshotData(Set<String> sstFilesToExclude, Path tmpdir, Set<Path> snapshotPaths,
Review Comment:
Changing method visibility from private to package-private breaks encapsulation. Consider creating a separate testable method or using reflection in tests to avoid exposing internal implementation details.
```suggestion
private void transferSnapshotData(Set<String> sstFilesToExclude, Path tmpdir, Set<Path> snapshotPaths,
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -476,7 +487,7 @@ private boolean writeDBToArchive(Set<String> sstFilesToExclude, Path dbDir, Atom
* @return The created database checkpoint.
* @throws IOException If an error occurs during checkpoint creation or file copying.
*/
- private DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean flush) throws IOException {
+ DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean flush) throws IOException {
Review Comment:
Changing method visibility from private to package-private breaks encapsulation. Consider creating a separate testable method or using reflection in tests to avoid exposing internal implementation details.
```suggestion
private DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean flush) throws IOException {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427992946
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
+ } finally {
+ if (checkpointMetadataManager != null) {
+ checkpointMetadataManager.stop();
+ }
Review Comment:
> OmMetadataManagerImpl implements AutoCloseable
no it doesn't
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
+ } finally {
+ if (checkpointMetadataManager != null) {
+ checkpointMetadataManager.stop();
+ }
Review Comment:
> OmMetadataManagerImpl implements AutoCloseable
no it doesn't
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427981928
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
Review Comment:
OmMetadataManager does not in any way extend AutoClosable so the try-with-resource block is not useful here
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #9131:
URL: https://github.com/apache/ozone/pull/9131#issuecomment-3404399009
Thanks @sadanand48 . Could you rebase the patch?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2427392662
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
Review Comment:
what if a snapshot is purged later during transferSnapshotData()?
looks like if a snapshot directory is deleted it will simply ignored.
So it looks to be it's okay if a snapshot is purged. But what if a new snapshot is created during bootstrap? Would it be missed?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465324733
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -398,6 +404,29 @@ Set<Path> getSnapshotDirs(OMMetadataManager omMetadataManager) throws IOExceptio
return snapshotPaths;
}
+ /**
+ * Collects paths to all snapshot databases from the OM DB.
+ *
+ * @param omMetadataManager OMMetadataManager instance
+ * @return Set of paths to snapshot databases
+ * @throws IOException if an I/O error occurs
+ */
+ Set<Path> getSnapshotDirsFromDB(OMMetadataManager omMetadataManager) throws IOException {
+ Set<Path> snapshotPaths = new HashSet<>();
+ try (TableIterator<String, ? extends Table.KeyValue<String, SnapshotInfo>> iter =
+ omMetadataManager.getSnapshotInfoTable().iterator()) {
+ while (iter.hasNext()) {
+ Table.KeyValue<String, SnapshotInfo> kv = iter.next();
+ SnapshotInfo snapshotInfo = kv.getValue();
+ String snapshotDir = OmSnapshotManager.getSnapshotPath(getConf(),
Review Comment:
```suggestion
boolean flushed = RatisHelper.attemptUntilTrue(() -> {
try {
return OmSnapshotManager.isSnapshotFlushedToDB(ozoneManager.getMetadataManager(), snapshotInfo);
} catch (IOException e) {
return false;
}
}, POLL_INTERVAL_DURATION, maxPollDuration);
String snapshotDir = OmSnapshotManager.getSnapshotPath(getConf(),
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2465455123
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -586,6 +592,119 @@ public void testBootstrapLockBlocksMultipleServices() throws Exception {
assertTrue(servicesSucceeded.get() > 0, "Services should have succeeded after lock release");
}
+ /**
+ * Tests the full checkpoint servlet flow to ensure snapshot paths are read
+ * from checkpoint metadata (frozen state) rather than live OM metadata (current state).
+ * Scenario:
+ * 1. Create snapshots S1, S2
+ * 2. Create Snapshot S3
+ * 3. Checkpoint is created (freezes state with S1, S2)
+ * 4. S2 gets purged from live OM (after checkpoint creation)
+ * 5. Servlet processes checkpoint - should still include S2 data
+ */
+ @Test
+ public void testCheckpointIncludesSnapshotsFromFrozenState() throws Exception {
Review Comment:
nit: Do we need a mini ozone cluster test case for this? I believe we can do with a unit test case here instead of a full mini ozone cluster test. We can think about moving this test into unit test later
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9131:
URL: https://github.com/apache/ozone/pull/9131#issuecomment-3478000143
@sadanand48 Let us create a follow up jira for changing the test to initialize the lock from the Servlet object itself instead of creating a new instance. Here we are not testing whether the InodeBasedCheckpointServlet instance is actually using the correct implementation of BootstrapLock or not
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13772. Snapshot Paths to be re read from om checkpoint db inside lock again. [ozone]
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2447785223
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap, false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(), checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org