Arik Hadas has posted comments on this change. Change subject: core: Numa engine/vdsm integration patch ......................................................................
Patch Set 7: (12 comments) looks ok to me, minor comments http://gerrit.ovirt.org/#/c/27083/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 85: private EngineLock monitoringLock; Line 86: Line 87: private final List<VdsNumaNode> saveNodeList = new ArrayList<>(); Line 88: private final List<VdsNumaNode> updateNodeList = new ArrayList<>(); Line 89: private final List<Guid> removeNodeList = new ArrayList<>(); 1. those fields are used only in updateNumaData, please make them local variables instead of class members 2. please rename them: numaNudesToSave, numaNodesToUpdate, numaNodesToRemove Line 90: public Object getLockObj() { Line 91: return _lockObj; Line 92: } Line 93: Line 391: * Line 392: * @param vds Line 393: */ Line 394: public void updateNumaData(VDS vds) { Line 395: List<VdsNumaNode> vdsNumaNodes = vds.getNumaNodeList(); please inline vdsNumaNodes field Line 396: List<VdsNumaNode> dbVdsNumaNodes = DbFacade.getInstance() Line 397: .getVdsNumaNodeDAO().getAllVdsNumaNodeByVdsId(vds.getId()); Line 398: for (VdsNumaNode node : vdsNumaNodes) { Line 399: VdsNumaNode searchNode = getVdsNumaNodeByIndex(dbVdsNumaNodes, node.getIndex()); Line 411: removeNodeList.add(node.getId()); Line 412: } Line 413: //need use _vds to be invoked in the below internal class Line 414: VDS backup = _vds; Line 415: _vds = vds; I don't understand why we need this backup+restore thing, you just need access to the id of the VDS, which is the same, right? can we set the vds argument as final and remove this part? Line 416: //The database operation should be in one transaction Line 417: TransactionSupport.executeInScope(TransactionScopeOption.Required, Line 418: new TransactionMethod<Void>() { Line 419: @Override http://gerrit.ovirt.org/#/c/27083/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java: Line 110: private final List<Guid> _vmsMovedToDown = new ArrayList<>(); Line 111: private final List<Guid> _vmsToRemoveFromAsync = new ArrayList<>(); Line 112: private final List<Guid> _succededToRunVms = new ArrayList<>(); Line 113: private final List<CpuStatistics> cpuStatisticsToSave = new ArrayList<>(); Line 114: private final List<VdsNumaNode> vdsNumaNodesToSave = new ArrayList<>(); please use local variables instead Line 115: private static final Map<Guid, Integer> vmsWithBalloonDriverProblem = new HashMap<>(); Line 116: private static final Map<Guid, Integer> vmsWithUncontrolledBalloon = new HashMap<>(); Line 117: private final List<VmStatic> _externalVmsToAdd = new ArrayList<>(); Line 118: private boolean _saveVdsDynamic; Line 275: for (CpuStatistics cpuStat : dbCpuStats) { Line 276: cpuStatsMap.put(cpuStat.getCpuId(), cpuStat); Line 277: } Line 278: for (CpuStatistics cpuStat : cpuStatisticsToSave) { Line 279: if (!cpuStatsMap.containsKey(cpuStat.getCpuId())) { you don't really need a map, you don't use the mapping since you never get values from the map. please replace it with HashSet<Guid>. I suggests to extract 269-284 to separate method 'isRemoveAndSaveNeeded' since this logic doesn't directly belongs to saveCpuStatisticsDataToDb and just makes it longer Line 280: needRemoveAndSave = true; Line 281: break; Line 282: } Line 283: } http://gerrit.ovirt.org/#/c/27083/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 772: } Line 773: vds.getNumaNodeList().clear(); Line 774: vds.getNumaNodeList().addAll(vdsNumaNodes); Line 775: vds.getStatisticsData().getCpuCoreStatistics().clear(); Line 776: vds.getStatisticsData().getCpuCoreStatistics().addAll(cpuStatsData); this method is too long: can you please extract #726-#733 to separate method which returns CpuStatistics and #741-#756 to separate method Line 777: } Line 778: Line 779: /** Line 780: * Update {@link VDS#setLocalDisksUsage(Map)} with map of paths usage extracted from the returned returned value. The Line 1569: } Line 1570: Line 1571: vds.getDynamicData().setNumaNodeList(newNumaNodeList); Line 1572: if (newNumaNodeList.size() > 1) { Line 1573: vds.setNumaSupport(true); can we replace it with one line: vds.setNumaSupport(newNumaNodeList.size() > 1); ? Line 1574: } Line 1575: } Line 1576: Line 1577: } Line 1575: } Line 1576: Line 1577: } Line 1578: Line 1579: private static VdsNumaNode getVdsNumaNodeByIndex(List<VdsNumaNode> numaNodes, int index) { looks familiar :) please put it in VdsHandler class or other Utils class, and call it from all the relevant places Line 1580: for (VdsNumaNode numaNode : numaNodes) { Line 1581: if (index == numaNode.getIndex()) { Line 1582: return numaNode; Line 1583: } http://gerrit.ovirt.org/#/c/27083/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java: Line 225: for (VdsNumaNode node : vdsNodes) { Line 226: vdsNumaNodeCpus.put(node.getIndex(), node.getCpuIds()); Line 227: } Line 228: log.error(vdsNumaNodeCpus.toString()); Line 229: List<Integer> pinnedNodeIndexList = null; please remove this declaration and declare it in #231 (and remove the List postfix, it should be pinnedNodeIndexes or something similar) Line 230: for (VmNumaNode node : vmNodes) { Line 231: pinnedNodeIndexList = getPinnedNodeIndexList(node.getVdsNumaNodeList()); Line 232: if (!pinnedNodeIndexList.isEmpty()) { Line 233: Set <Integer> totalPinnedVdsCpus = new LinkedHashSet<>(); Line 263: nodeIndexes.add(item.getIndex()); Line 264: } Line 265: return nodeIndexes; Line 266: } Line 267: return new ArrayList<Integer>(); classic place to use Collections.emptyList() Line 268: } Line 269: Line 270: private List<Integer> getPinnedNodeIndexList(List<Pair<Guid, Pair<Boolean, Integer>>> nodeList) { Line 271: if (!nodeList.isEmpty()) { Line 269: Line 270: private List<Integer> getPinnedNodeIndexList(List<Pair<Guid, Pair<Boolean, Integer>>> nodeList) { Line 271: if (!nodeList.isEmpty()) { Line 272: List<Integer> nodeIndexes = new ArrayList<>(nodeList.size()); Line 273: for (Pair<Guid, Pair<Boolean, Integer>> item : nodeList) { how about to iterate the values so it will be a bit simpler code? Line 274: if (item.getSecond().getFirst()) { Line 275: nodeIndexes.add(item.getSecond().getSecond()); Line 276: } Line 277: } Line 276: } Line 277: } Line 278: return nodeIndexes; Line 279: } Line 280: return new ArrayList<Integer>(); classic place to use Collections.emptyList() Line 281: } Line 282: Line 283: protected void buildVmNetworkCluster() { Line 284: // set Display network -- To view, visit http://gerrit.ovirt.org/27083 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I050bc9d80a90ac73b5642ccd7630dd352eba236e Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[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
