Skip to content

HDDS-13901. OmSnaphshotLocalDataManager should throw IOException if unable to resolve to a previous snapshot id #9269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
swamirishi merged 2 commits into from
Nov 10, 2025
Merged

HDDS-13901. OmSnaphshotLocalDataManager should throw IOException if unable to resolve to a previous snapshot id #9269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3771ff4
HDDS-13901. OmSnaphshotLocalDataManager should throw IOException if u…
swamirishi Nov 9, 2025
c129207
Merge remote-tracking branch 'apache/master' into HEAD
swamirishi Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions ...manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,13 @@ private LockDataProviderInitResult initialize(
currentIteratedSnapshotId, snapId, toResolveSnapshotId));
}
UUID previousId = previousIds.iterator().next();
// If the previousId is null and if toResolveSnapshotId is not null then should throw an exception since
// the snapshot can never be resolved against the toResolveSnapshotId.
Comment on lines +660 to +661
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 660-661 is misleading in this context. At this point in the code, we're inside a while loop iterating through the chain, and previousId was just extracted from previousIds.iterator().next(). The comment states 'If the previousId is null' but this is checking after we've already verified previousIds is not empty (line 654). The comment would be more accurate if it stated 'If previousId is null after extracting from the set, it indicates the chain is broken and cannot continue to toResolveSnapshotId.'

Suggested change
// If the previousId is null and if toResolveSnapshotId is not null then should throw an exception since
// the snapshot can never be resolved against the toResolveSnapshotId.
// If previousId is null after extracting from the set, it indicates the chain is broken and cannot
// continue to toResolveSnapshotId.

Copilot uses AI. Check for mistakes.
if (previousId == null) {
throw new IOException(String.format(
"Snapshot %s versions previousId is null thus %s cannot be resolved against id %s",
currentIteratedSnapshotId, snapId, toResolveSnapshotId));
}
HierarchicalResourceLock previousToPreviousReadLockAcquired = acquireLock(previousId, true);
try {
// Get the version node for the snapshot and update the version node to the successor to point to the
Expand Down Expand Up @@ -705,6 +712,12 @@ private LockDataProviderInitResult initialize(
// Set the previous snapshot version to the relativePreviousVersionNode which was captured.
versionMeta.setPreviousSnapshotVersion(relativePreviousVersionNode.getVersion());
}
} else if (toResolveSnapshotId != null) {
// If the previousId is null and if toResolveSnapshotId is not null then should throw an exception since
// the snapshot can never be resolved against the toResolveSnapshotId.
Comment on lines +716 to +717
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 716-717 is inaccurate for this code location. This check is in an 'else if' block that only executes when toResolveSnapshotId != null (as per line 715), so the comment's condition 'if toResolveSnapshotId is not null' is redundant. More importantly, this block is reached when previousSnapshotId == null (from line 625, since line 631's condition failed), not when 'previousId' is null. The comment should be updated to: 'If the current snapshot's previousSnapshotId is null but toResolveSnapshotId is not null, throw an exception since the snapshot cannot be resolved against toResolveSnapshotId.'

Suggested change
// If the previousId is null and if toResolveSnapshotId is not null then should throw an exception since
// the snapshot can never be resolved against the toResolveSnapshotId.
// If the current snapshot's previousSnapshotId is null but toResolveSnapshotId is not null,
// throw an exception since the snapshot cannot be resolved against toResolveSnapshotId.

Copilot uses AI. Check for mistakes.
throw new IOException(String.format("Unable to resolve previous snapshot id for snapshot: %s against " +
"previous snapshotId : %s since current snapshot's previousSnapshotId is null",
snapId, toResolveSnapshotId));
} else {
toResolveSnapshotId = null;
ssLocalData.setPreviousSnapshotId(null);
Expand Down
38 changes: 24 additions & 14 deletions ...ger/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotLocalDataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ public void testVersionResolution(boolean read) throws IOException {
addVersionsToLocalData(localDataManager, snapshotIds.get(i), versionMaps.get(i));
}
for (int start = 0; start < snapshotIds.size(); start++) {
for (int end = start + 1; end < snapshotIds.size(); end++) {
for (int end = 0; end < snapshotIds.size(); end++) {
UUID prevSnapId = snapshotIds.get(start);
UUID snapId = snapshotIds.get(end);
Map<Integer, Integer> versionMap = new HashMap<>(versionMaps.get(end));
Expand All @@ -695,19 +695,29 @@ public void testVersionResolution(boolean read) throws IOException {
version.setValue(versionMaps.get(idx).getOrDefault(version.getValue(), 0));
}
}
try (ReadableOmSnapshotLocalDataProvider snap = read ?
localDataManager.getOmSnapshotLocalData(snapId, prevSnapId) :
localDataManager.getWritableOmSnapshotLocalData(snapId, prevSnapId)) {
OmSnapshotLocalData snapshotLocalData = snap.getSnapshotLocalData();
OmSnapshotLocalData prevSnapshotLocalData = snap.getPreviousSnapshotLocalData();
assertEquals(prevSnapshotLocalData.getSnapshotId(), snapshotLocalData.getPreviousSnapshotId());
assertEquals(prevSnapId, snapshotLocalData.getPreviousSnapshotId());
assertEquals(snapId, snapshotLocalData.getSnapshotId());
assertTrue(snapshotLocalData.getVersionSstFileInfos().size() > 1);
snapshotLocalData.getVersionSstFileInfos()
.forEach((version, versionMeta) -> {
assertEquals(versionMap.get(version), versionMeta.getPreviousSnapshotVersion());
});
if (start >= end) {
assertThrows(IOException.class, () -> {
if (read) {
localDataManager.getOmSnapshotLocalData(snapId, prevSnapId);
} else {
localDataManager.getWritableOmSnapshotLocalData(snapId, prevSnapId);
}
});
} else {
try (ReadableOmSnapshotLocalDataProvider snap = read ?
localDataManager.getOmSnapshotLocalData(snapId, prevSnapId) :
localDataManager.getWritableOmSnapshotLocalData(snapId, prevSnapId)) {
OmSnapshotLocalData snapshotLocalData = snap.getSnapshotLocalData();
OmSnapshotLocalData prevSnapshotLocalData = snap.getPreviousSnapshotLocalData();
assertEquals(prevSnapshotLocalData.getSnapshotId(), snapshotLocalData.getPreviousSnapshotId());
assertEquals(prevSnapId, snapshotLocalData.getPreviousSnapshotId());
assertEquals(snapId, snapshotLocalData.getSnapshotId());
assertTrue(snapshotLocalData.getVersionSstFileInfos().size() > 1);
snapshotLocalData.getVersionSstFileInfos()
.forEach((version, versionMeta) -> {
assertEquals(versionMap.get(version), versionMeta.getPreviousSnapshotVersion());
});
}
}
}
}
Expand Down
Loading