Copilot commented on code in PR #9079:
URL: https://github.com/apache/ozone/pull/9079#discussion_r3049102758
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1180,6 +1199,37 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
// TODO: [SNAPSHOT] Refactor the parameter list
Optional<Set<String>> deltaFiles = Optional.empty();
+ OmSnapshotLocalData fromSnapshotLocalData = getSnapshotLocalData(fsInfo);
+ int fromSnapshotVersion = fromSnapshotLocalData.getVersion();
+ OmSnapshotLocalData toSnapshotLocalData = getSnapshotLocalData(tsInfo);
+ int toSnapshotVersion = toSnapshotLocalData.getVersion();
+
+ try {
+ if (fromSnapshotVersion > 0 && toSnapshotVersion > 0) {
+ // both snapshots are defragmented, To calculate snap-diff, we can
simply compare the
+ // SST files contained in OmSnapshotLocalData instances of both these
and get the delta files
+ OmSnapshotLocalData.VersionMeta toSnapVersionMeta =
+
toSnapshotLocalData.getVersionSstFileInfos().get(toSnapshotVersion);
+ // get the source snapshot SST files (versionMeta) corresponding to
target snapshot
+ OmSnapshotLocalData.VersionMeta fromSnapVersionMeta =
+ resolveBaseVersionMeta(toSnapshotLocalData,
fromSnapshot.getSnapshotID());
+ // Calculate diff files using helper method
+ if (toSnapVersionMeta == null) {
+ String errMsg =
+ "Cannot find corresponding version of from snapshot " +
fromSnapshotVersion + " from " + tsInfo;
+ LOG.error(errMsg);
+ throw new IOException(errMsg);
+ }
+ List<String> diffFiles = calculateDiffFiles(fromSnapVersionMeta,
toSnapVersionMeta);
+ return OmSnapshotUtils.getSSTDiffListWithFullPath(diffFiles,
+
OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(),
fsInfo).toString(),
+
OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(),
tsInfo).toString(), diffDir);
Review Comment:
`getDeltaFiles` now unconditionally reads snapshot-local YAML via
`getSnapshotLocalData(fsInfo/tsInfo)` before entering the try/catch and before
checking `fsInfo/tsInfo` for null. If either SnapshotInfo is null (which this
method already guards for later) or if YAML loading throws, the exception will
propagate and prevent the intended fallback to DAG/full-diff paths. Consider
moving these calls inside the try block and guarding them with `fsInfo != null
&& tsInfo != null` so failures fall back cleanly.
```suggestion
try {
if (fsInfo != null && tsInfo != null) {
OmSnapshotLocalData fromSnapshotLocalData =
getSnapshotLocalData(fsInfo);
int fromSnapshotVersion = fromSnapshotLocalData.getVersion();
OmSnapshotLocalData toSnapshotLocalData =
getSnapshotLocalData(tsInfo);
int toSnapshotVersion = toSnapshotLocalData.getVersion();
if (fromSnapshotVersion > 0 && toSnapshotVersion > 0) {
// both snapshots are defragmented, To calculate snap-diff, we can
simply compare the
// SST files contained in OmSnapshotLocalData instances of both
these and get the delta files
OmSnapshotLocalData.VersionMeta toSnapVersionMeta =
toSnapshotLocalData.getVersionSstFileInfos().get(toSnapshotVersion);
// get the source snapshot SST files (versionMeta) corresponding
to target snapshot
OmSnapshotLocalData.VersionMeta fromSnapVersionMeta =
resolveBaseVersionMeta(toSnapshotLocalData,
fromSnapshot.getSnapshotID());
// Calculate diff files using helper method
if (toSnapVersionMeta == null) {
String errMsg =
"Cannot find corresponding version of from snapshot " +
fromSnapshotVersion + " from " + tsInfo;
LOG.error(errMsg);
throw new IOException(errMsg);
}
List<String> diffFiles = calculateDiffFiles(fromSnapVersionMeta,
toSnapVersionMeta);
return OmSnapshotUtils.getSSTDiffListWithFullPath(diffFiles,
OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(),
fsInfo).toString(),
OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(),
tsInfo).toString(), diffDir);
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1223,6 +1273,81 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
+ /**
+ * Resolve the VersionMeta of the ancestor snapshot (fromSnapshotId)
+ * that the given snapshot (toSnapshot) was built on.
+ * Traverses the snapshot chain backwards using prevSnapId.
+ *
+ * @param toSnapshot the target snapshot.
+ * @param fromSnapshotId UUID of the source snapshot.
+ * @return the resolved VersionMeta of the source that was used to build the
target.
+ */
+ private OmSnapshotLocalData.VersionMeta resolveBaseVersionMeta(
+ OmSnapshotLocalData toSnapshot,
+ UUID fromSnapshotId) throws IOException {
+ OmMetadataManagerImpl metadataManager =
+ (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ // Start walking back from the child snapshot
+ OmSnapshotLocalData child = toSnapshot;
+ int numSnapshotsTraversed = 0;
+ while (numSnapshotsTraversed <= maxSnapshotLimit &&
!child.getPreviousSnapshotId().equals(fromSnapshotId)) {
+ UUID parentId = child.getPreviousSnapshotId();
+ // Load the parent snapshot in the chain
+ child = getSnapshotLocalData(
+ getSnapshotInfo(ozoneManager,
+ metadataManager.getSnapshotChainManager(),
+ parentId));
Review Comment:
`resolveBaseVersionMeta` calls
`child.getPreviousSnapshotId().equals(fromSnapshotId)` in the loop condition.
Since snapshot local YAML can now have a null previousSnapshotId, this can
throw NPE and spam error logs during normal operation. Add explicit handling
for `child.getPreviousSnapshotId() == null` (eg return null to trigger
fallback) and consider using `numSnapshotsTraversed < maxSnapshotLimit` to
avoid the current off-by-one traversal behavior.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1180,6 +1199,37 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
// TODO: [SNAPSHOT] Refactor the parameter list
Optional<Set<String>> deltaFiles = Optional.empty();
+ OmSnapshotLocalData fromSnapshotLocalData = getSnapshotLocalData(fsInfo);
+ int fromSnapshotVersion = fromSnapshotLocalData.getVersion();
+ OmSnapshotLocalData toSnapshotLocalData = getSnapshotLocalData(tsInfo);
+ int toSnapshotVersion = toSnapshotLocalData.getVersion();
+
+ try {
+ if (fromSnapshotVersion > 0 && toSnapshotVersion > 0) {
+ // both snapshots are defragmented, To calculate snap-diff, we can
simply compare the
+ // SST files contained in OmSnapshotLocalData instances of both these
and get the delta files
+ OmSnapshotLocalData.VersionMeta toSnapVersionMeta =
+
toSnapshotLocalData.getVersionSstFileInfos().get(toSnapshotVersion);
+ // get the source snapshot SST files (versionMeta) corresponding to
target snapshot
+ OmSnapshotLocalData.VersionMeta fromSnapVersionMeta =
+ resolveBaseVersionMeta(toSnapshotLocalData,
fromSnapshot.getSnapshotID());
+ // Calculate diff files using helper method
+ if (toSnapVersionMeta == null) {
+ String errMsg =
+ "Cannot find corresponding version of from snapshot " +
fromSnapshotVersion + " from " + tsInfo;
Review Comment:
The error message when `toSnapVersionMeta == null` is misleading: it
mentions "corresponding version of from snapshot" but the null being checked is
the *to* snapshot's VersionMeta for `toSnapshotVersion`, and it prints
`fromSnapshotVersion` rather than `toSnapshotVersion`. Updating the message
(and including both snapshot IDs/versions) would make diagnosing
YAML/version-map issues much easier.
```suggestion
"Cannot find VersionMeta for to snapshot [id="
+ toSnapshot.getSnapshotID() + ", version=" +
toSnapshotVersion
+ "] while calculating diff from from snapshot [id="
+ fromSnapshot.getSnapshotID() + ", version="
+ fromSnapshotVersion + "], fsInfo=" + fsInfo
+ ", tsInfo=" + tsInfo;
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -1651,4 +1685,242 @@ public void testGetSnapshotDiffReportJob() throws
Exception {
}
}
}
+
+ @Test
+ public void testGetDeltaFilesWithDefragmentedSnapshotsSameVersion() throws
IOException {
+ // Setup test snapshots
+ UUID fromSnapId = UUID.randomUUID();
+ UUID toSnapId = UUID.randomUUID();
+ String fromSnapName = "fromSnap-" + fromSnapId;
+ String toSnapName = "toSnap-" + toSnapId;
+ // Create mock snapshots
+ OmSnapshot fromSnapshot = getMockedOmSnapshot(fromSnapId);
+ OmSnapshot toSnapshot = getMockedOmSnapshot(toSnapId);
+ SnapshotInfo fromSnapshotInfo = getSnapshotInfoInstance(VOLUME_NAME,
BUCKET_NAME, fromSnapName, fromSnapId);
+ SnapshotInfo toSnapshotInfo = getSnapshotInfoInstance(VOLUME_NAME,
BUCKET_NAME, toSnapName, toSnapId);
+ when(ozoneManager.getMetadataManager()).thenReturn(omMetadataManagerImpl);
+ // Create mock local data with version 1 for both snapshots
+ // fromSnapshot has: [common.sst]
+ // toSnapshot has: [common.sst, unique_to.sst]
+ // Expected diff: [unique_to.sst]
+ OmSnapshotLocalData fromLocalData = createMockVersionedLocalData(1,
+ Arrays.asList("common.sst"), null, 0);
+ OmSnapshotLocalData toLocalData = createMockVersionedLocalData(1,
+ Arrays.asList("common.sst", "unique_to.sst"), fromSnapId, 1);
+ SnapshotDiffManager spy = spy(snapshotDiffManager);
+ // Mock dependencies
+ doNothing().when(spy).recordActivity(any(), any());
+ doNothing().when(spy).updateProgress(anyString(), anyDouble());
+ doReturn(fromLocalData).when(spy).getSnapshotLocalData(fromSnapshotInfo);
+ doReturn(toLocalData).when(spy).getSnapshotLocalData(toSnapshotInfo);
+
+ String diffJobKey = fromSnapId + DELIMITER + toSnapId;
+ String diffDir = snapDiffDir.getAbsolutePath();
+
+ try (MockedStatic<OmSnapshotUtils> mockedUtils =
mockStatic(OmSnapshotUtils.class);
+ MockedStatic<OmSnapshotManager> mockedManager =
mockStatic(OmSnapshotManager.class);
+ MockedStatic<SnapshotUtils> snapshotUtilsMockedStatic =
mockStatic(SnapshotUtils.class)) {
+ // Mock path resolution
+ mockedManager.when(
+ () ->
OmSnapshotManager.getSnapshotPath(any(OmMetadataManagerImpl.class),
eq(fromSnapshotInfo)))
+ .thenReturn(snapDiffDir.toPath().resolve("from"));
+ mockedManager.when(() ->
OmSnapshotManager.getSnapshotPath(any(OmMetadataManagerImpl.class),
eq(toSnapshotInfo)))
+ .thenReturn(snapDiffDir.toPath().resolve("to"));
+ snapshotUtilsMockedStatic.when(() -> SnapshotUtils.getSnapshotInfo(
+ eq(ozoneManager),
+ any(),
+ eq(fromSnapId))).thenReturn(fromSnapshotInfo);
+
+ snapshotUtilsMockedStatic.when(() -> SnapshotUtils.getSnapshotInfo(
+ eq(ozoneManager),
+ any(),
+ eq(toSnapId))).thenReturn(toSnapshotInfo);
+ // Expected diff files: files that are different between snapshots
+ Set<String> expectedDiffFiles = Sets.newHashSet("unique_to.sst");
+ // Mock the utility method to return the expected diff files
+ mockedUtils.when(
+ () ->
OmSnapshotUtils.getSSTDiffListWithFullPath(eq(Arrays.asList("unique_to.sst")),
anyString(), anyString(),
+ eq(diffDir))).thenReturn(expectedDiffFiles);
+ // Execute the method under test
+ Set<String> result = spy.getDeltaFiles(
+ fromSnapshot,
+ toSnapshot,
+ Arrays.asList("cf1", "cf2"),
+ fromSnapshotInfo,
+ toSnapshotInfo,
+ false, // useFullDiff = false to test optimized path
+ Collections.emptyMap(),
+ diffDir,
+ diffJobKey);
+ // Verify results - should contain exactly the 2 different files
Review Comment:
This assertion block says "should contain exactly the 2 different files",
but the test data/expectations are for a single diff file (`unique_to.sst`) and
`result.size()` is asserted to be 1. Please update the comment to match the
actual expectation to avoid confusion when maintaining the test.
```suggestion
// Verify results - should contain exactly the expected single
different file
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotUtils.java:
##########
@@ -214,4 +215,69 @@ public static void linkFiles(File oldDir, File newDir)
throws IOException {
}
}
}
+
+ /**
+ * Locates the full path of the specified SST file by searching the backup
directory
+ * first, followed by the provided database directories.
+ *
+ * @param sstFilename The name of the SST file to locate.
+ * @param sstBackupDir The directory where SST backups are stored.
+ * @param dbPaths One or more database directories to search for the
SST file.
+ * @return The full path to the SST file as a string.
+ * @throws RuntimeException if the SST file cannot be found in any of the
specified directories.
+ */
+ private static String getSSTFullPath(String sstFilename, String sstBackupDir,
+ String... dbPaths) {
+
+ // Try to locate the SST in the backup dir first
+ final Path sstPathInBackupDir = Paths.get(sstBackupDir,
+ sstFilename);
+ if (Files.exists(sstPathInBackupDir)) {
+ return sstPathInBackupDir.toString();
+ }
+
+ // SST file does not exist in the SST backup dir, this means the SST file
+ // has not gone through any compactions yet and is only available in the
+ // src DB directory or destDB directory
+ for (String dbPath : dbPaths) {
+ final Path sstPathInDBDir = Paths.get(dbPath,
+ sstFilename);
+ if (Files.exists(sstPathInDBDir)) {
+ return sstPathInDBDir.toString();
+ }
+ }
+
+ // TODO: More graceful error handling?
+ throw new RuntimeException("Unable to locate SST file: " +
+ sstFilename);
+ }
Review Comment:
`getSSTFullPath` throws a `RuntimeException` with only the filename when it
can't locate an SST. Since this is used in snapshot diff execution paths, the
resulting failure is hard to diagnose. Consider throwing an `IOException` (or
at least including all searched directories in the message) so callers can log
actionable context and/or fall back predictably.
--
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]
