Liran Zelkha 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. Done 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 ? Done 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. Done 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 We have a naming convention - the name of the query is the same as the name of the calling method. It appears in the VdsStaticDaoDbFacade - so I rather keep the same name. 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 definiti Done 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 ? Since VmDynamic hols a list of VdsNumaNode, doing both as EAGER will result in an Exception (Can't fetch Multiple Bags - JPA limitation since it will create a cartesian product). 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 on Done 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 ? 1. No... Very bummer. 2. Fixed 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 Done 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 Done 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 tes It was copied from the Dao, so even this is an improvement. Can we do it in another push? Specifically - I don't even know how to test these... 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 It can be removed altogether. Fixed. -- 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
