Arik Hadas has posted comments on this change.
Change subject: core: introduce customized snapshot preview support
......................................................................
Patch Set 6:
(5 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 246:
Line 247: getParameters().setImagesList((List<DiskImage>)
CollectionUtils.union(imagesFromPreviewSnapshot, intersection));
Line 248: imagesToRestore = imagesFromPreviewSnapshot;
Line 249: updateSnapshotIdForSkipRestoreImages(
Line 250:
ImagesHandler.imagesSubtract(imagesFromActiveSnapshot, imagesToRestore),
targetSnapshot.getId());
it's a bit confusing, because when restoring from stateless snapshot all the
disks should restored to their previous active volumes. it will be better that
the code would reflect that.
Line 251: break;
Line 252:
Line 253: // Currently UI sends the "in preview" snapshot to restore
when "Commit" is pressed.
Line 254: case REGULAR:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 176: for (DiskImage image : filteredImages) {
Line 177: VdcReturnValueBase vdcReturnValue =
Line 178:
Backend.getInstance().runInternalAction(VdcActionType.TryBackToSnapshot,
Line 179:
buildTryBackToSnapshotParameters(newActiveSnapshotId, image),
Line 180:
ExecutionHandler.createDefaultContexForTasks(getExecutionContext()));
why not taking a snapshot for the "excluded" disks as well (which is based on
the active volume) just in case the preview will lead bad things on those disks?
Line 181:
Line 182: if (vdcReturnValue.getSucceeded()) {
Line 183:
getTaskIdList().addAll(vdcReturnValue.getInternalVdsmTaskIdList());
Line 184: } else if (vdcReturnValue.getFault() != null)
{
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
Line 462: vm.setImages(imagesFromVmConf);
Line 463: }
Line 464:
Line 465: return true;
Line 466: }
1. why calling VM#setImages in each iteration and not just one time in the end?
I see that if we return false, it will be set in
attempToRestoreVmConfigurationFromSnapshot later anyway
2. the call to to vmFromConf.getImages() returns the volumes or the images in
PDIV ? because if that's the images then we can breat after #459, but I'm not
sure if that's not the volumes..
Line 467:
Line 468: /**
Line 469: * @param vmDevice
Line 470: * @return true if the device can be removed (disk which allows
snapshot can be removed as it is part
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java
Line 7:
Line 8: public class RestoreAllSnapshotsParameters extends
TryBackToAllSnapshotsOfVmParameters implements java.io.Serializable {
Line 9: private static final long serialVersionUID = -8756081739745132849L;
Line 10:
Line 11: private List<DiskImage> images;
to conform the standard, either change it to imagesList or change the
getter+setter (I think the second option is better, it's obvious that it is a
list from the type)
Line 12:
Line 13: public RestoreAllSnapshotsParameters(Guid vmId, Guid
dstSnapshotId) {
Line 14: super(vmId, dstSnapshotId);
Line 15: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/TryBackToAllSnapshotsOfVmParameters.java
Line 45: public List<DiskImage> getDisks() {
Line 46: return disks;
Line 47: }
Line 48:
Line 49: public void setDisks(List<DiskImage> images) {
images -> disks
Line 50: this.disks = images;
Line 51: }
--
To view, visit http://gerrit.ovirt.org/22776
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89332a1538c7259dd3ea62d693ac062f47e52cd0
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
