Copilot commented on code in PR #9269:
URL: https://github.com/apache/ozone/pull/9269#discussion_r2511783489
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java:
##########
@@ -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.
Review Comment:
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.'
```suggestion
// If the current snapshot's previousSnapshotId is null but
toResolveSnapshotId is not null,
// throw an exception since the snapshot cannot be resolved
against toResolveSnapshotId.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java:
##########
@@ -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.
Review Comment:
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.'
```suggestion
// If previousId is null after extracting from the set, it
indicates the chain is broken and cannot
// continue to toResolveSnapshotId.
```
--
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]
