Roy Golan has posted comments on this change. Change subject: engine: NUMA feature queries and actions validation ......................................................................
Patch Set 8: (10 comments) http://gerrit.ovirt.org/#/c/27617/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java: Line 29: protected VmNumaNodeDAO getVmNumaNodeDao() { Line 30: return getDbFacade().getVmNumaNodeDAO(); Line 31: } Line 32: Line 33: protected boolean checkNumaValid() { for better readability please separate blocks of code with new line Line 34: List<VmNumaNode> vmNumaNodes = getParameters().getVmNumaNodeList(); Line 35: if (vmNumaNodes == null || vmNumaNodes.size() == 0) { Line 36: // if VM do not contain any NUMA node, skip checking Line 37: return true; Line 34: List<VmNumaNode> vmNumaNodes = getParameters().getVmNumaNodeList(); Line 35: if (vmNumaNodes == null || vmNumaNodes.size() == 0) { Line 36: // if VM do not contain any NUMA node, skip checking Line 37: return true; Line 38: } new line here Line 39: VM vm = getVm(); Line 40: boolean pinHost = vm.getMigrationSupport() == MigrationSupport.PINNED_TO_HOST; Line 41: Guid vdsId = vm.getDedicatedVmForVds(); Line 42: if (pinHost && vdsId == null) { Line 40: boolean pinHost = vm.getMigrationSupport() == MigrationSupport.PINNED_TO_HOST; Line 41: Guid vdsId = vm.getDedicatedVmForVds(); Line 42: if (pinHost && vdsId == null) { Line 43: return failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NOT_EXIST); Line 44: } new line here Line 45: List<VdsNumaNode> vdsNumaNodes = new ArrayList<>(); Line 46: if (pinHost) { Line 47: vdsNumaNodes = getVdsNumaNodeDao().getAllVdsNumaNodeByVdsId(vdsId); Line 48: if (vdsNumaNodes == null || vdsNumaNodes.size() == 0) { Line 47: vdsNumaNodes = getVdsNumaNodeDao().getAllVdsNumaNodeByVdsId(vdsId); Line 48: if (vdsNumaNodes == null || vdsNumaNodes.size() == 0) { Line 49: return failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NODE_EMPTY); Line 50: } Line 51: } new line here Line 52: boolean memStrict = vm.getNumaTuneMode() == NumaTuneMode.STRICT; Line 53: for (VmNumaNode vmNumaNode : vmNumaNodes) { Line 54: for (Pair<Guid, Pair<Boolean, Integer>> pair : vmNumaNode.getVdsNumaNodeList()) { Line 55: if (pair == null || pair.getSecond() == null || pair.getSecond().getSecond() == null) { Line 55: if "pair" can't be null, its inside a forEach loop Line 53: for (VmNumaNode vmNumaNode : vmNumaNodes) { Line 54: for (Pair<Guid, Pair<Boolean, Integer>> pair : vmNumaNode.getVdsNumaNodeList()) { Line 55: if (pair == null || pair.getSecond() == null || pair.getSecond().getSecond() == null) { Line 56: return failCanDoAction(VdcBllMessages.VM_NUMA_NODE_PINNED_INDEX_ERROR); Line 57: } new line here Line 58: Integer index = pair.getSecond().getSecond(); Line 59: for (VdsNumaNode vdsNumaNode : vdsNumaNodes) { Line 60: if (vdsNumaNode.getIndex() == index) { Line 61: if (memStrict && vmNumaNode.getMemTotal() > vdsNumaNode.getMemTotal()) { http://gerrit.ovirt.org/#/c/27617/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AddVmNumaNodesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AddVmNumaNodesCommand.java: Line 23: boolean succeeded = false; Line 24: try { Line 25: Guid vmId = getParameters().getVmId(); Line 26: List<VmNumaNode> vmNumaNodes = getParameters().getVmNumaNodeList(); Line 27: if (vmNumaNodes != null && vmNumaNodes.size() > 0) { that have been checked already in cando action Line 28: Guid vdsId = getVm().getDedicatedVmForVds(); Line 29: List<VdsNumaNode> vdsNumaNodes = new ArrayList<>(); Line 30: if (vdsId != null) { Line 31: vdsNumaNodes = getVdsNumaNodeDao().getAllVdsNumaNodeByVdsId(vdsId); Line 51: succeeded = true; Line 52: } finally { Line 53: setSucceeded(succeeded); Line 54: } Line 55: } these 4 lines including line 23 and the try-finally should be one line. by default, the returnValue.getSucceeded is false. so if every thing is ok, just setSucceeded(true) Line 56: Line 57: @Override Line 58: protected boolean canDoAction() { Line 59: return checkNumaValid(); Line 53: setSucceeded(succeeded); Line 54: } Line 55: } Line 56: Line 57: @Override checkNumaValid should be the canDoAction override. Line 58: protected boolean canDoAction() { Line 59: return checkNumaValid(); Line 60: } Line 61: http://gerrit.ovirt.org/#/c/27617/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java: Line 21: @Override Line 22: protected void executeCommand() { Line 23: boolean succeeded = false; Line 24: try { Line 25: List<VmNumaNode> vmNumaNodes = getParameters().getVmNumaNodeList(); all comments about AddVmNuma command are valid here as well Line 26: if (vmNumaNodes != null && vmNumaNodes.size() > 0) { Line 27: Guid vdsId = getVm().getDedicatedVmForVds(); Line 28: List<VdsNumaNode> vdsNumaNodes = new ArrayList<>(); Line 29: if (vdsId != null) { -- To view, visit http://gerrit.ovirt.org/27617 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c299405ec5d82ada713ed3d220554bf3055c145 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jason Liao <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jason Liao <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Xiaolei Shi <[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
