Moti Asayag has posted comments on this change. Change subject: core: Make VDS a JPA entity ......................................................................
Patch Set 58: (12 comments) https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaNodeStatistics.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NumaNodeStatistics.java: Line 103: if (!(obj instanceof NumaNodeStatistics)) { Line 104: return false; Line 105: } Line 106: NumaNodeStatistics other = (NumaNodeStatistics) obj; Line 107: return Objects.equals(cpuIdle, other.cpuIdle) && Objects.equals(cpuSys, other.cpuSys) the formatter should format as a single condition per line. Line 108: && Objects.equals(cpuUsagePercent, other.cpuUsagePercent) && Objects.equals(cpuUser, other.cpuUser) Line 109: && Objects.equals(memFree, other.memFree) && Objects.equals(memUsagePercent, other.memUsagePercent); Line 110: } Line 111: https://gerrit.ovirt.org/#/c/36600/58/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 254: @Column(name = "supported_emulated_machines") Line 255: private String supportedEmulatedMachines; Line 256: Line 257: @OneToMany(cascade = CascadeType.ALL) Line 258: @LazyCollection(LazyCollectionOption.FALSE) why not use FetchType.EAGER instead of LazyCollectionOption.FALSE ? Line 259: @JoinColumn(name = "vds_id", referencedColumnName = "vds_id") Line 260: private List<VdsNumaNode> numaNodeList; Line 261: Line 262: @Column(name = "auto_numa_balancing") Line 651: return null; Line 652: } Line 653: StringBuilder sb = new StringBuilder(); Line 654: for (Version version : versionSet) { Line 655: sb.append(version.toString()); there is no separator between the versions. would you mind replacing this with : for (int i = 0 ; i < versionSet.size() ; i++) { sb.append(version.toString()); if (i < versionSet.size() - 1) { sb.append(','); } } return sb.toString(); In addition, it seems that this will become common around, maybe it should be pulled into new class, say VersionUtils ? (the class could be added next time there is a need for this). Line 656: } Line 657: if (sb.length() > 0) { Line 658: sb.deleteCharAt(sb.length() - 1); Line 659: } https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsStatic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsStatic.java: Line 36: @Cacheable(true) Line 37: @NamedQueries({ Line 38: @NamedQuery(name = "VdsStatic.getByHostName", Line 39: query = "select v from VdsStatic v where v.hostName = :hostName"), Line 40: @NamedQuery(name = "VdsStatic.getAllForVdsGroup", s/getAllForVdsGroup/getAllForCluster Line 41: query = "select v from VdsStatic v where v.vdsGroupId = :vdsGroupId"), Line 42: @NamedQuery(name = "VdsStatic.getByVdsName", Line 43: query = "select v.name from VdsStatic v where v.name = :name") Line 44: }) https://gerrit.ovirt.org/#/c/36600/58/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 31: @Type(type = "org.ovirt.engine.core.dao.jpa.GuidUserType") Line 32: private Guid id; Line 33: Line 34: @Column(name = "cpu_idle") Line 35: private BigDecimal cpuIdle; please format. there should be a space line between every variable definition. Line 36: @Column(name = "cpu_load") Line 37: private BigDecimal cpuLoad; Line 38: @Column(name = "cpu_sys") Line 39: private BigDecimal cpuSys; https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmNumaNode.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmNumaNode.java: Line 27: Line 28: private static final long serialVersionUID = -5384287037435972730L; Line 29: Line 30: @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, mappedBy = "vmNumaNode") Line 31: @LazyCollection(LazyCollectionOption.FALSE) why not FetchType.EAGER instead ? Line 32: private List<NumaNodeVmVds> numaNodeVdsList; Line 33: Line 34: public VmNumaNode() { Line 35: setNumaNodeVdsList(new ArrayList<NumaNodeVmVds>()); https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsStaticDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsStaticDAODbFacadeImpl.java: Line 57: Line 58: @Override Line 59: public void update(VdsStatic vds) { Line 60: Guid id = vds.getId(); Line 61: if (Guid.isNullOrEmpty(id)) { why should that ever happen ? this kind of logic seems relevant for save only. Line 62: id = Guid.newGuid(); Line 63: vds.setId(id); Line 64: } Line 65: super.update(vds); https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java: Line 29: Line 30: @Override Line 31: public List<VmNumaNode> getAllPinnedVmNumaNodeByVdsNumaNodeId(Guid vdsNumaNodeId) { Line 32: List<VmNumaNode> vmNumaNodes = Line 33: multipleResults(entityManager.createNativeQuery("select * from GetVmNumaNodeByVdsNumaNodeIdWithPinnedInfo(CAST( CAST (? AS text) AS uuid),?)", 1. isn't there a nicer way to provide Guid as parameter to a named query ? why the double casting is needed ? 2. could you split the query into few lines ? it exceeds line's limit Line 34: VmNumaNode.class) Line 35: .setParameter(1, vdsNumaNodeId.toString()) Line 36: .setParameter(2, true)); Line 37: Line 40: Line 41: @Override Line 42: public List<VmNumaNode> getAllVmNumaNodeByVdsNumaNodeId(Guid vdsNumaNodeId) { Line 43: List<VmNumaNode> vmNumaNodes = Line 44: multipleResults(entityManager.createNativeQuery("select * from GetVmNumaNodeByVdsNumaNodeId(CAST( CAST (? AS text) AS uuid))", same here Line 45: VmNumaNode.class) Line 46: .setParameter(1, vdsNumaNodeId.toString())); Line 47: Line 48: return vmNumaNodes; Line 51: @Override Line 52: public List<Pair<Guid, VmNumaNode>> getVmNumaNodeInfoByVdsGroupId(Guid vdsGroupId) { Line 53: List<Pair<Guid, VmNumaNode>> vmNumaNodes = new ArrayList<>(); Line 54: List<VmNumaNode> nodes = Line 55: multipleResults(entityManager.createNativeQuery("select vm_numa_node_id as numa_node_id, vm_numa_node_index as numa_node_index,vm_numa_node_mem_total as mem_total, vm_numa_node_distance as distance, vm_numa_node_cpu_idle as cpu_idle, vm_numa_node_cpu_sys as cpu_sys, vm_numa_node_usage_cpu_percent as usage_cpu_percent, vm_numa_node_cpu_user as cpu_user, vm_numa_node_mem_free as mem_free, vm_numa_node_usage_mem_percent as usage_mem_percent, null as vds_id, vm_numa_node_vm_id as vm_id from GetVmNumaNodeByVdsGroup(CAST( CAST (? AS text) AS uuid))", same Line 56: VmNumaNode.class) Line 57: .setParameter(1, vdsGroupId.toString())); Line 58: Line 59: for (VmNumaNode node : nodes) { https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/jpa/DistanceUserType.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/jpa/DistanceUserType.java: Line 96: public int[] sqlTypes() { Line 97: return new int[] { Types.VARCHAR }; Line 98: } Line 99: Line 100: // format: (<index_id>, <distance>);*, for example: "0, 10; 2, 16" i think such code should be stored on utils library and be covered with tests (preferably @Parameterized runner). Line 101: private Map<Integer, Integer> getDistanceMap(String distance) { Line 102: Map<Integer, Integer> nodeDistance = new HashMap<>(); Line 103: if (StringUtils.isBlank(distance)) { Line 104: return nodeDistance; https://gerrit.ovirt.org/#/c/36600/58/backend/manager/modules/dal/src/test/resources/logging.properties File backend/manager/modules/dal/src/test/resources/logging.properties: Line 13: org.ovirt.engine.core.level=INFO Line 14: org.ovirt.engine.core.utils.ejb.EJBUtilsStrategy.level=SEVERE Line 15: org.ovirt.engine.core.dal.dbbroker.PostgresDbEngineDialect$PostgresJdbcTemplate.level=WARNING Line 16: org.hibernate.SQL.level=FINEST Line 17: org.hibernate.type.level=FINEST isn't that for debug purpose and should be removed/tuned to a higher level ? -- To view, visit https://gerrit.ovirt.org/36600 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5d7aa140e20dcd9468ae1ed00af1df0cf3e8b9e6 Gerrit-PatchSet: 58 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
