Gilad Chaplik has posted comments on this change. Change subject: engine: Numa feature entities ......................................................................
Patch Set 12: (25 comments) 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 12: public class CpuStatistics implements Serializable { Line 13: Line 14: private static final long serialVersionUID = 3274786304152401306L; Line 15: Line 16: private Guid id; > It's the primary key in database. Cpu statistics are stored independently i disagree, but maybe useful someday. although I like to code what's needed. please rethink about it. Line 17: Line 18: private Guid vdsId; Line 19: Line 20: private Guid vdsNumaNodeId; Line 14: private static final long serialVersionUID = 3274786304152401306L; Line 15: Line 16: private Guid id; Line 17: Line 18: private Guid vdsId; > I think it's needed. Since we store cpu's statistics in a database table, n will this object be separated from the its container? which is already identified, if no this is data duplication. Line 19: Line 20: private Guid vdsNumaNodeId; Line 21: Line 22: private int cpuCoreId; Line 16: private Guid id; Line 17: Line 18: private Guid vdsId; Line 19: Line 20: private Guid vdsNumaNodeId; > Need this information to calculate the cpu statistics of each NUMA node. you didn't reply my question, NUMA Node type is UUID? isn't it a number/index? Line 21: Line 22: private int cpuCoreId; Line 23: Line 24: private BigDecimal cpuSys; Line 24: private BigDecimal cpuSys; Line 25: Line 26: private BigDecimal cpuUser; Line 27: Line 28: private BigDecimal cpuIdle; > Here we refer to the definition of VdsStatistics. please reply 'Done.' if you agree with a comment. Line 29: Line 30: private int cpuUsagePercent; Line 31: Line 32: public CpuStatistics(){ 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 9: /** Line 10: * Object which represents vm virtual NUMA node information Line 11: * Line 12: */ Line 13: public class GuestNumaNode implements Serializable { > GuestNumaNode is for virtual NUMA node of vm, NumaNode is for host NUMA nod disagree. please exact a base class (one of them) or create a new one. All the following comments will be taken care of once you use inheritance (###): Line 14: Line 15: private static final long serialVersionUID = 3751844852367478102L; Line 16: Line 17: private Guid id; Line 13: public class GuestNumaNode implements Serializable { Line 14: Line 15: private static final long serialVersionUID = 3751844852367478102L; Line 16: Line 17: private Guid id; > Database primary key. ### Line 18: Line 19: private Guid vmGuid; Line 20: Line 21: private int index; Line 15: private static final long serialVersionUID = 3751844852367478102L; Line 16: Line 17: private Guid id; Line 18: Line 19: private Guid vmGuid; > But in database, it's saved in a independent table, need this property for ### Line 20: Line 21: private int index; Line 22: Line 23: private String vcpuIds; Line 19: private Guid vmGuid; Line 20: Line 21: private int index; Line 22: Line 23: private String vcpuIds; > use to calculate cpupin when put a vmNumaNode into vdsNumaNode. ### Line 24: Line 25: private int vcpuCount; Line 26: Line 27: private long memTotal; Line 25: private int vcpuCount; Line 26: Line 27: private long memTotal; Line 28: Line 29: private List<NumaNode> pinnedNumaNodeList; > Will change to vdsNumaNodeId, each vmNumaNode only pin to a single vdsNuma no. a vNUMA Node can be splitted across several NumaNodes. Line 30: Line 31: public GuestNumaNode() { Line 32: vcpuCount = 0; Line 33: memTotal = 0L; Line 28: Line 29: private List<NumaNode> pinnedNumaNodeList; Line 30: Line 31: public GuestNumaNode() { Line 32: vcpuCount = 0; > leave it as it is, unless Roy checks with Vojtech and verifies it's not nee here is my original comment. please restore all c'tors and initialization of private fields 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 11: public class NumaNode implements Serializable { Line 12: Line 13: private static final long serialVersionUID = -683066053231559224L; Line 14: Line 15: private Guid id; > Database primary key. same, please rethink. Line 16: Line 17: private Guid vdsGuid; Line 18: Line 19: private int index; Line 13: private static final long serialVersionUID = -683066053231559224L; Line 14: Line 15: private Guid id; Line 16: Line 17: private Guid vdsGuid; > for query same, please remove. Line 18: Line 19: private int index; Line 20: Line 21: private String cpuIds; Line 17: private Guid vdsGuid; Line 18: Line 19: private int index; Line 20: Line 21: private String cpuIds; > to calculate the cpu pin when drag vmNumaNode into vdsNumaNode. can you explain what is the format of the string, give examples or something. I don't follow. Line 22: Line 23: private int cpuCount; Line 24: Line 25: private long memTotal; Line 26: Line 27: private int vNumaNodeCount; Line 28: Line 29: public NumaNode(){ Line 30: cpuCount = 0; > Will remove same, please restore Line 31: memTotal = 0L; Line 32: vNumaNodeCount = 0; Line 33: } Line 34: 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>(); > The value will be persisted. We just follow the general style to define thi I agree, Roy, you're asking a general change, please consult with lists. Line 17: for (NumaTuneMode mode : values()) { Line 18: mappings.put(mode.getValue(), mode); Line 19: } Line 20: } Line 19: } Line 20: } Line 21: Line 22: private NumaTuneMode() { Line 23: value= name().toLowerCase(); > this value will be set to libvirt, the lower case is required. I've asked for formatting and you did, don't understand what you've replied on :) Line 24: } Line 25: Line 26: private NumaTuneMode(String value) { Line 27: this.value = value; 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(); > Can you explain which type fields should be in static, and which in dynamic lets stay focused on NUMA: IMO NUMA host topology is static, VM's is dynamic. Line 1319: } Line 1320: Line 1321: public List<NumaNode> getNumaNodeList() { Line 1322: return mVdsDynamic.getNumaNodeList(); Line 1330: return mVdsStatistics.getCpuStatisticsList(); Line 1331: } Line 1332: Line 1333: public String getNumaNodeDistance() { Line 1334: return mVdsDynamic.getNumaNodeDistance(); > Will add. ? also why the distance isn't in the node class as well? Line 1335: } Line 1336: Line 1337: public Boolean getAutoNumaBalancing() { Line 1338: return mVdsDynamic.getAutoNumaBalancing(); Line 1333: public String getNumaNodeDistance() { Line 1334: return mVdsDynamic.getNumaNodeDistance(); Line 1335: } Line 1336: Line 1337: public Boolean getAutoNumaBalancing() { > Use object Boolean because this value may be null if we can't get it from h okay, still missing javadoc Line 1338: return mVdsDynamic.getAutoNumaBalancing(); Line 1339: } Line 1340: http://gerrit.ovirt.org/#/c/23702/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java: Line 1715: return this.vmStatic.getvNumaNodeCount(); Line 1716: } Line 1717: Line 1718: public List<NumaNode> getNumaTuneNodeList() { Line 1719: return this.vmStatic.getNumaTuneNodeList(); > A scenario, a vm may be pinned to several vdsNumaNode without create vmNuma still don't get it. in this scenario the VM will reside on a single vNUMA node. anything looks the same. Line 1720: } Line 1721: Line 1722: public List<GuestNumaNode> getvNumaNodeList() { Line 1723: return this.vmStatic.getvNumaNodeList(); Line 1723: return this.vmStatic.getvNumaNodeList(); Line 1724: } Line 1725: Line 1726: public boolean isNumaManualBinding() { Line 1727: return this.vmStatic.isNumaManualBinding(); > Will add. still don't get it. please elaborate Line 1728: } Line 1729: Line 1730: public void setNumaManualBinding(boolean numaManualBinding) { Line 1731: this.vmStatic.setNumaManualBinding(numaManualBinding); 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 150: private int numaNodeCount; Line 151: Line 152: private List<NumaNode> numaNodeList; Line 153: Line 154: private String numaNodeDistance; > will add. the javadoc should on public getters in container class. still think this info should reside in NumaNode Line 155: Line 156: private Boolean autoNumaBalancing; Line 157: Line 158: public void setVersion(RpmVersion value) { Line 152: private List<NumaNode> numaNodeList; Line 153: Line 154: private String numaNodeDistance; Line 155: Line 156: private Boolean autoNumaBalancing; > will add. public getter, container class. Line 157: Line 158: public void setVersion(RpmVersion value) { Line 159: rpmVersion = value; Line 160: } Line 187: vm_count = 0; Line 188: vms_cores_count = 0; Line 189: guest_overhead = 0; Line 190: powerManagementControlledByPolicy = false; Line 191: numaNodeCount = 0; > will remove I think I've commented about it. keep common usage, if the class init private native fields you should do it as well. and I saw you removed a c'tor (cpu_stats iirc), retrieve that code please. Roy, you can take it to the lists or submit a patch for it. 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/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: > Will add annotation. please address my comments as well. please remove annotations, add them in a later patch. 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: Vinzenz Feenstra <vfeen...@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
