Maor Lipchuk has posted comments on this change. Change subject: core: handle 'source' domains when adding a vm from template ......................................................................
Patch Set 5: (2 comments) http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 422: if (!verifySourceDomains() || !buildAndCheckDestStorageDomains()) { Line 423: return false; Line 424: } Line 425: Line 426: chooseDisksSourceDomains(); > and just to add - i'm pretty sure that your suggestion will lead to findbug What will prevent the next programmer to move this method to some other place in the CDA, how will he knows that it is dependent on verifySourceDomains? That will just make the code much less maintenanble. maybe the following approach might solve the find bug issue you mentioned: if (verifySourceDomains() && buildAndCheckDestStorageDomains()) { chooseDisksSourceDomains() } else { return false; } Line 427: Line 428: // otherwise.. Line 429: storageToDisksMap = Line 430: ImagesHandler.buildStorageToDiskMap(getImagesToCheckDestinationStorageDomains(), http://gerrit.ovirt.org/#/c/26262/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java: Line 187: } Line 188: } Line 189: Line 190: private Guid retrieveDestinationDomainForDisk(Guid id) { Line 191: return diskInfoDestinationMap.get(id).getStorageIds().get(0); > this is not wrong, the disks in diskInfoDestinationMap contains on the Stor but you added lines 182-186 and a comment which describing that the same storage domain should be chosen for possibly faster operation, although it is looks that it just picks the first storage domain for each disk every time Line 192: } Line 193: Line 194: @Override Line 195: protected boolean isVirtioScsiEnabled() { -- To view, visit http://gerrit.ovirt.org/26262 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8d02cfded41a3588dc944d9dfcd5a3eec88c45ab Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
