Copilot commented on code in PR #12949:
URL: https://github.com/apache/cloudstack/pull/12949#discussion_r3056369475
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java:
##########
@@ -111,7 +111,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
FreezeThawVMAnswer freezeAnswer = null;
FreezeThawVMCommand thawCmd = null;
FreezeThawVMAnswer thawAnswer = null;
- List<SnapshotInfo> forRollback = new ArrayList<>();
+ List<SnapshotInfo> snapshotInfoListForRollback = new ArrayList<>();
Review Comment:
`snapshotInfoListForRollback` is very verbose and repeats the generic type
(`SnapshotInfo`) already conveyed by `List<SnapshotInfo>`. Consider renaming to
something shorter and intention-revealing like `rollbackSnapshots` /
`snapshotsForRollback` to improve readability (optional).
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java:
##########
@@ -448,15 +454,14 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot
vmSnapshot, List<SnapshotIn
vol.addPayload(setPayload(vol, snapshot, quiescevm));
SnapshotInfo snapshotInfo =
snapshotDataFactory.getSnapshot(snapshot.getId(), vol.getDataStore());
snapshotInfo.addPayload(vol.getpayload());
+ snapshotInfoListForRollback.add(snapshotInfo);
SnapshotStrategy snapshotStrategy =
storageStrategyFactory.getSnapshotStrategy(snapshotInfo,
SnapshotOperation.TAKE);
Review Comment:
This change alters rollback semantics by adding the snapshot to the rollback
list before strategy lookup and `takeSnapshot()`. Please add/extend a unit test
to assert that when snapshot creation fails (e.g., `getSnapshotStrategy()`
returns null or `takeSnapshot()` throws/returns null), the created snapshot is
still included in the rollback list and rollback cleanup is triggered as
expected.
--
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]
