Maor Lipchuk has posted comments on this change. Change subject: engine: Add action type for previewed snapshot. ......................................................................
Patch Set 11: (6 comments) http://gerrit.ovirt.org/#/c/20420/11//COMMIT_MSG Commit Message: Line 6: Line 7: engine: Add action type for previewed snapshot. Line 8: Line 9: Adding an enum which distinguish between Undo, Commit Line 10: and Restore from stateless operations > What do you mean by stateless operations? I meant "Restore from Stateless" as an operation by it self. I will re-pharse it that it will be more clear Line 11: on snapshot. Line 12: Line 13: Change-Id:If877befc5058c3412ae3d3731d5beacbc09e5c97 Line 14: Related to Bug-Url: https://bugzilla.redhat.com/970115 http://gerrit.ovirt.org/#/c/20420/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java: Line 141: case RESTORE_STATELESS: Line 142: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.STATELESS); Line 143: break; Line 144: default: Line 145: log.error("No valid Snapshot Action has been transferred"); > I would throw an exception here, because all possible enum values are liste agreed regarding printing the action, I will add it. Regarding the exception, I disagree, it will not have much benefit IMHO Line 146: } Line 147: Line 148: if (snapshot != null) { Line 149: snapshotId = snapshot.getId(); Line 422: !validate(storageValidator.allDomainsWithinThresholds()) || Line 423: !performImagesChecks() || Line 424: !validate(vmValidator.vmDown()) || Line 425: // if the user choose to commit a snapshot - the vm cant have disk snapshots attached to other vms. Line 426: (getSnapshotDao().get(getSnapshotId()).getType() == SnapshotType.REGULAR && !validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms(false)))) { > Don't you already fetched the snapshot in the previous method? I did, I just changed getParameters().getDstSnapshotId() to getSnapshotId(). This should be in another patch, I will change it Line 427: return false; Line 428: } Line 429: Line 430: Snapshot snapshot = getSnapshotDao().get(getSnapshotId()); http://gerrit.ovirt.org/#/c/20420/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java: Line 6: import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotAction; Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class RestoreAllSnapshotsParameters extends VmOperationParameterBase implements java.io.Serializable { Line 10: private static final long serialVersionUID = -8756081739745132849L; > Why we need this if we don't regenerate it after the class was changed? if??? which if? Line 11: Line 12: private List<DiskImage> images; Line 13: private SnapshotAction snapshotAction; Line 14: Line 21: return images; Line 22: } Line 23: Line 24: public void setImages(List<DiskImage> images) { Line 25: this.images = images; > Is this method in use? I removed the constructor since it was not in use. The setImages is still being used from other commands Line 26: } Line 27: Line 28: public RestoreAllSnapshotsParameters() { Line 29: } http://gerrit.ovirt.org/#/c/20420/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Snapshot.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Snapshot.java: Line 287: return typeName; Line 288: } Line 289: } Line 290: Line 291: public enum SnapshotAction { > should be static done Not related but perhaps SnapshotType should also be changed to static then Line 292: UNDO, Line 293: COMMIT, Line 294: RESTORE_STATELESS Line 295: } -- To view, visit http://gerrit.ovirt.org/20420 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If877befc5058c3412ae3d3731d5beacbc09e5c97 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michael Pasternak <mishka8...@yahoo.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
