Tomas Jelinek has posted comments on this change. Change subject: core: clone VM ......................................................................
Patch Set 7: (5 comments) http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java: Line 485: } Line 486: Line 487: protected abstract VM getSourceVmFromDb(); Line 488: Line 489: protected void unlockEntities() { > it looks like this method and lockEntities are not used right? if so it sho It is called in this class. It is something like the template method pattern. Normally this two methods would be abstract but I did not want to have an empty implementation in CloneVmCommand... Line 490: Line 491: } Line 492: protected void lockEntities() { Line 493: http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java: Line 200: } Line 201: Line 202: Map<String, Pair<String, String>> union = new HashMap<>(); Line 203: union.putAll(parentLocks); Line 204: union.putAll(thisLocks); > isn't it better to just add the 'thisLocks' to 'parentLocks' map in a simil well, the parent returns a singletonMap which is immutable. I could create the "thisLocs" as mutable and add the parentLocs to thisLocs, but I personally find it a bit more readable to prepare the two well named maps and than have a union map called "union" and return that. Line 205: Line 206: return union; Line 207: } Line 208: http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CloneVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CloneVmCommand.java: Line 75: LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK, getDiskSharedLockMessage())); Line 76: } Line 77: Line 78: locks.put(getSourceVmFromDb().getId().toString(), Line 79: LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); > we shouldn't use this deprecated message anymore especially for locks which Done Line 80: Line 81: return locks; Line 82: } Line 83: Line 97: } Line 98: return diskImagesFromConfiguration; Line 99: } Line 100: Line 101: public Map<String, String> getJobMessageProperties() { > missing @overrides Done Line 102: if (jobProperties == null) { Line 103: jobProperties = super.getJobMessageProperties(); Line 104: jobProperties.put(VdcObjectType.VM.name().toLowerCase(), Line 105: StringUtils.defaultString(getParameters().getNewName())); http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcActionUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcActionUtils.java: Line 148: VdcActionType.AddVmInterface, VdcActionType.UpdateVmInterface, Line 149: VdcActionType.RemoveVmInterface, VdcActionType.CreateAllSnapshotsFromVm, Line 150: VdcActionType.ExtendImageSize, VdcActionType.RebootVm)); Line 151: vmMatrix.put( Line 152: VMStatus.Suspended, > on second thought, it is not that good to enable the clone VM operation for good point, you are right, disabling. Line 153: EnumSet.of(VdcActionType.HibernateVm, VdcActionType.AddVmTemplate, Line 154: VdcActionType.RunVmOnce, VdcActionType.MigrateVm, VdcActionType.ExportVm, VdcActionType.MoveVm, Line 155: VdcActionType.ImportVm, VdcActionType.ChangeDisk, VdcActionType.RemoveVm, Line 156: VdcActionType.AddVmInterface, VdcActionType.UpdateVmInterface, -- To view, visit http://gerrit.ovirt.org/23805 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3038638314e399e9d2390e7204ce4cc00e85e6ae Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [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
