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

[swamirishi]

What changes were proposed in this pull request?

Currently if a snapshot is not able to resolve to a particular previous snapshot then instead of throwing an IOException it is resolved to null which may be wrong when used along with snapshot diff. Snapshot diff flow could happen b/w two snapshots where the fromSnapshot could be more recent than toSnapshot.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13901

How was this patch tested?

Added unit tests

[Copilot]

Pull Request Overview

This PR addresses an issue where OmSnapshotLocalDataManager incorrectly handled cases when a snapshot cannot be resolved to a particular previous snapshot ID. Previously, the code would silently allow null resolutions, which could lead to incorrect behavior in snapshot diff operations (e.g., when fromSnapshot is more recent than toSnapshot). The fix adds proper validation to throw IOExceptions in these cases.

Key changes:

• Added validation to throw IOException when previousId is null during snapshot chain traversal
• Added validation to throw IOException when toResolveSnapshotId is not null but current snapshot's previousSnapshotId is null
• Extended test coverage to validate both valid (start < end) and invalid (start >= end) snapshot resolution scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
OmSnapshotLocalDataManager.java	Added two validation checks to throw IOException when snapshot resolution fails: one during chain traversal when previousId is null (lines 660-666), and one when the initial previousSnapshotId is null (lines 715-720)
TestOmSnapshotLocalDataManager.java	Modified test to cover both valid and invalid snapshot resolution cases by changing the loop bounds and adding assertion for IOException when start >= end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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.

[jojochuang]

LGTM

[swamirishi]

thank you @jojochuang for reviewing the patch

[adoroszlai]

@swamirishi please set fix version when resolving Jira issue after PR merge