Roy Golan has posted comments on this change. Change subject: engine: Numa feature entities ......................................................................
Patch Set 12: (10 comments) thanks for posting. initial comments inside. in general, CPU topology reporting is under work, thus we all should be in sync - make sure you add Vinzenz to your patches both backend/vdsm http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/CpuStatistics.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/CpuStatistics.java: Line 98: public void setCpuUsagePercent(int cpuUsagePercent) { Line 99: this.cpuUsagePercent = cpuUsagePercent; Line 100: } Line 101: Line 102: @Override is really needed? Line 103: public int hashCode() { Line 104: final int prime = 31; Line 105: int result = 1; Line 106: result = prime * result + cpuCoreId; Line 114: return result; Line 115: } Line 116: Line 117: @Override Line 118: public boolean equals(Object obj) { is really needed? Line 119: if (this == obj) Line 120: return true; Line 121: if (obj == null) Line 122: return false; http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GuestNumaNode.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GuestNumaNode.java: Line 28: Line 29: private List<NumaNode> pinnedNumaNodeList; Line 30: Line 31: public GuestNumaNode() { Line 32: vcpuCount = 0; primitives int defaults to zero so this is redundant Line 33: memTotal = 0L; Line 34: pinnedNumaNodeList = new ArrayList<NumaNode>(); Line 35: } Line 36: http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaNode.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaNode.java: Line 30: all are primitive fields. they are all 0 by default http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaTuneMode.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaTuneMode.java: Line 12: private String value; Line 13: private static Map<String, NumaTuneMode> mappings; Line 14: Line 15: static { Line 16: mappings = new HashMap<String, NumaTuneMode>(); valueOf of enum does the same. you can leave this enum with just the three members decleration. the rest is taken care by the enum itself. Line 17: for (NumaTuneMode mode : values()) { Line 18: mappings.put(mode.getValue(), mode); Line 19: } Line 20: } http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java: Line 1314: mVdsStatic.setDisablePowerManagementPolicy(disablePowerManagementPolicy); Line 1315: } Line 1316: Line 1317: public int getNumaNodeCount() { Line 1318: return mVdsDynamic.getNumaNodeCount(); > I'm for numa in VdsStatic. @Gilad explain your point Line 1319: } Line 1320: Line 1321: public List<NumaNode> getNumaNodeList() { Line 1322: return mVdsDynamic.getNumaNodeList(); http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java: Line 187: vm_count = 0; Line 188: vms_cores_count = 0; Line 189: guest_overhead = 0; Line 190: powerManagementControlledByPolicy = false; Line 191: numaNodeCount = 0; pls don't init a primitive type field Line 192: numaNodeList = new ArrayList<NumaNode>(); Line 193: } Line 194: Line 195: public Integer getcpu_cores() { http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsStatistics.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsStatistics.java: Line 36: private boolean highlyAvailableIsActive; Line 37: private boolean highlyAvailableGlobalMaintenance; Line 38: private boolean highlyAvailableLocalMaintenance; Line 39: Line 40: private List<NumaNodeStatistics> numaNodeStatisticsList; please document what is it - its not ordinary to see Lists in VdsStatistics Line 41: Line 42: private List<CpuStatistics> cpuStatisticsList; Line 43: Line 44: public VdsStatistics() { Line 39: Line 40: private List<NumaNodeStatistics> numaNodeStatisticsList; Line 41: Line 42: private List<CpuStatistics> cpuStatisticsList; Line 43: the same here. Line 44: public VdsStatistics() { Line 45: this.cpu_idle = BigDecimal.ZERO; Line 46: this.cpu_load = BigDecimal.ZERO; Line 47: this.cpu_sys = BigDecimal.ZERO; http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java: Line 55: private Guid imageTypeId; Line 56: Line 57: @EditableField Line 58: private boolean useLatestVersion; Line 59: if all those fields should be editable (which I guess they should) then they must have @EditableField annotation. - if NOt I'd suggest to encapsulate all below in an object, say Numa.java Line 60: private boolean numaManualBinding; Line 61: Line 62: private NumaTuneMode numaTuneMode; Line 63: -- To view, visit http://gerrit.ovirt.org/23702 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifefade432e7955503980bdc6fc5d73ea32818a95 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi <xiao-lei....@hp.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Eli Mesika <elimes...@gmail.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa <gustavo.pedr...@eldorado.org.br> Gerrit-Reviewer: Jiří Moskovčák <jmosk...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Vitor de Lima <vitor.l...@eldorado.org.br> 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
