Omer Frenkel has posted comments on this change. Change subject: core: Add migration options to OVF ......................................................................
Patch Set 4: (5 comments) i didnt check all references yet http://gerrit.ovirt.org/#/c/32899/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 2309: * @param isQuietMode Line 2310: * - don't write errors messages (used for cases where the caller fixes the problem and continues). Line 2311: * @return Line 2312: */ Line 2313: protected boolean isDedicatedVdsExistOnSameCluster(VmBase vm, Boolean isQuietMode) { this method doesn't belong to command base, if you need it accessible from multiple commands, maybe move it to VmHandler, there it would probably be static and you will pass the canDoActionMessages as parameter, and 'quiteMode' will just be null in this param Line 2314: boolean result = true; Line 2315: if (vm.getDedicatedVmForVds() != null) { Line 2316: // get dedicated host id Line 2317: Guid guid = vm.getDedicatedVmForVds(); Line 2311: * @return Line 2312: */ Line 2313: protected boolean isDedicatedVdsExistOnSameCluster(VmBase vm, Boolean isQuietMode) { Line 2314: boolean result = true; Line 2315: if (vm.getDedicatedVmForVds() != null) { just a suggestion, when the method is short, you can consider 'early returns' it usually save the nesting, for example: Guid vdsId = vm.getDedicatedVmForVds(); if (vdsId == null) { return true; } .. if (vds == null) { .. return false; } .. if (..) { return false; } return true; } it is a matter of style so up to you Line 2316: // get dedicated host id Line 2317: Guid guid = vm.getDedicatedVmForVds(); Line 2318: // get dedicated host, checks if exists and compare its cluster to the VM cluster Line 2319: VDS vds = getVdsDAO().get(guid); Line 2313: protected boolean isDedicatedVdsExistOnSameCluster(VmBase vm, Boolean isQuietMode) { Line 2314: boolean result = true; Line 2315: if (vm.getDedicatedVmForVds() != null) { Line 2316: // get dedicated host id Line 2317: Guid guid = vm.getDedicatedVmForVds(); vdsId would be a better name Line 2318: // get dedicated host, checks if exists and compare its cluster to the VM cluster Line 2319: VDS vds = getVdsDAO().get(guid); Line 2320: if (vds == null) { Line 2321: if(!isQuietMode) { http://gerrit.ovirt.org/#/c/32899/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 1048: getVm().getStaticData().setMinAllocatedMem(computeMinAllocatedMem()); Line 1049: getVm().getStaticData().setQuotaId(getParameters().getQuotaId()); Line 1050: Line 1051: // if "run on host" field points to a non existent vds (in the current cluster) -> remove field and continue Line 1052: if(!isDedicatedVdsExistOnSameCluster(getVm().getStaticData(), true)){ missing space: "if(" -> "if (" Line 1053: getVm().setDedicatedVmForVds(null); Line 1054: } Line 1055: Line 1056: if (getVm().getOriginalTemplateGuid() != null && !VmTemplateHandler.BLANK_VM_TEMPLATE_ID.equals(getVm().getOriginalTemplateGuid())) { http://gerrit.ovirt.org/#/c/32899/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java: Line 577: * @param vm Line 578: * - the VM to check Line 579: * @return Line 580: */ Line 581: protected boolean isDedicatedVdsExistOnSameCluster(VmStatic vm) { this method can be removed Line 582: boolean result = true; Line 583: if (vm.getDedicatedVmForVds() != null) { Line 584: // get dedicated host id Line 585: Guid guid = vm.getDedicatedVmForVds(); -- To view, visit http://gerrit.ovirt.org/32899 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f0025e5ac59dac0a1569729c2fef8186e0885db Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[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
