Xiaolei Shi has posted comments on this change. Change subject: core: Numa engine/vdsm integration patch ......................................................................
Patch Set 7: (12 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 var Done 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 Done 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 acc Done 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 Done 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 Done 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 metho Done 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() Done 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, a Done 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 Done 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() Done 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? Done 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() Done 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 <xiao-lei....@hp.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Xiaolei Shi <xiao-lei....@hp.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
