Vered Volansky has posted comments on this change. Change subject: core: change import vm to be VmCommand - part 4 ......................................................................
Patch Set 17: (3 comments) http://gerrit.ovirt.org/#/c/34788/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommandBase.java: Line 39: } Line 40: Line 41: /** Line 42: * Cloning a new disk and all its volumes with a new generated id.<br/> Line 43: * The disk will have the same parameters as <code>disk</code>.<br/> tags make this less readable IMO. Line 44: * Also adding the disk to <code>newDiskGuidForDisk</code> map, so we will be able to link between the new cloned disk Line 45: * and the old disk id. Line 46: * Line 47: * @param diskImagesList Line 48: * - All the disk volumes Line 49: * @param disk Line 50: * - The disk which is about to be cloned Line 51: */ Line 52: protected void generateNewDiskId(List<DiskImage> diskImagesList, DiskImage disk) { The name both here and bellow is misleading a makes me think this is just Id generation without reading the comments. I'd change the names to something like cloneDisk, cloneDiskWithNewId - on the latter (line 67), and to applyDiskIdToVolumes or something simillar to the first (line 52). Also, IMO, the comment about adding the disk to the map is enough in the later method that's actually doing it, though I can see why you'd want it here, so whatever you think is best. Line 53: Guid generatedGuid = generateNewDiskId(disk); Line 54: for (DiskImage diskImage : diskImagesList) { Line 55: diskImage.setId(generatedGuid); Line 56: } Line 74: return newGuidForDisk; Line 75: } Line 76: Line 77: /** Line 78: * Updating managed device map of VM, with the new disk {@link Guid}s.<br/> s in the end seems to be there by mistake. Line 79: * The update of managedDeviceMap is based on the newDiskIdForDisk map, Line 80: * so this method should be called only after newDiskIdForDisk is initialized. Line 81: * Line 82: * @param disk -- To view, visit http://gerrit.ovirt.org/34788 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f182417f9b64f23bb5f412a8e6e20e1c4e2ab6a Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
