Moti Asayag has posted comments on this change. Change subject: engine: Peer probing with alternate addresses ......................................................................
Patch Set 11: Code-Review-1 (9 comments) each new stored procedure must be followed with a dao unit test to cover it. https://gerrit.ovirt.org/#/c/38149/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSyncJob.java: Line 216: } Line 217: } Line 218: Line 219: private void peerProbeAlternateInterfaces(Network glusterNetwork, VDS host) { Line 220: GlusterServer glusterServer = getGlusterServerDao().get(host.getId()); you can split this condition, so in case glusterNetwork is null or the status isn't supported, quite early, without need to fetch the glusterServer from the db. it will also reduce the nesting levels, which improves readability. Line 221: if (glusterNetwork != null && host.getStatus() == VDSStatus.Up Line 222: && glusterServer != null) { Line 223: List<VdsNetworkInterface> interfaces = getInterfaceDao().getAllInterfacesForVds(host.getId()); Line 224: for (VdsNetworkInterface iface : interfaces) { Line 222: && glusterServer != null) { Line 223: List<VdsNetworkInterface> interfaces = getInterfaceDao().getAllInterfacesForVds(host.getId()); Line 224: for (VdsNetworkInterface iface : interfaces) { Line 225: if (glusterNetwork.getName().equals(iface.getNetworkName()) && Line 226: !StringUtils.isBlank(iface.getAddress()) could be replaced by StringUtils.isNotBlank() instead of negating the isBlank() Line 227: && !glusterServer.getKnownAddresses().contains(iface.getAddress())) { Line 228: // get another server in the cluster Line 229: VDS upServer = getAlternateUpServerInCluster(host.getVdsGroupId(), Line 230: host.getId()); Line 225: if (glusterNetwork.getName().equals(iface.getNetworkName()) && Line 226: !StringUtils.isBlank(iface.getAddress()) Line 227: && !glusterServer.getKnownAddresses().contains(iface.getAddress())) { Line 228: // get another server in the cluster Line 229: VDS upServer = getAlternateUpServerInCluster(host.getVdsGroupId(), the two lines could be merged by the formatter (they aren't exceed 120 chars) Line 230: host.getId()); Line 231: if (upServer != null) { Line 232: Boolean peerProbed = glusterPeerProbeAdditionalInterface(upServer.getId(), iface.getAddress()); Line 233: if (peerProbed) { Line 228: // get another server in the cluster Line 229: VDS upServer = getAlternateUpServerInCluster(host.getVdsGroupId(), Line 230: host.getId()); Line 231: if (upServer != null) { Line 232: Boolean peerProbed = glusterPeerProbeAdditionalInterface(upServer.getId(), iface.getAddress()); should be primitive boolean Line 233: if (peerProbed) { Line 234: getGlusterServerDao().addKnownAddress(host.getId(), iface.getAddress()); Line 235: } Line 236: } Line 252: Line 253: private VDS getAlternateUpServerInCluster(Guid clusterId, Guid vdsId) { Line 254: List<VDS> vdsList = getVdsDao().getAllForVdsGroupWithStatus(clusterId, VDSStatus.Up); Line 255: // If the cluster already having Gluster servers, get an up server Line 256: if (vdsList != null && vdsList.size() > 0) { vdsList cannot be null by the contract of the dao for fetching collections. Please replace with if (!vdsList.isEmpty()) Line 257: for (VDS vds : vdsList) { Line 258: if (!vdsId.equals(vds.getId())) { Line 259: return vds; Line 260: } https://gerrit.ovirt.org/#/c/38149/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterServer.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterServer.java: Line 11: private static final long serialVersionUID = -1425566208615075937L; Line 12: Line 13: private Guid serverId; Line 14: Line 15: private ArrayList<String> knownAddresses; can't it be just List ? Line 16: Line 17: private Guid glusterServerUuid; Line 18: Line 19: public GlusterServer() { https://gerrit.ovirt.org/#/c/38149/11/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterServerDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterServerDaoDbFacadeImpl.java: Line 48: GlusterServer glusterServer = new GlusterServer(); Line 49: glusterServer.setId(getGuidDefaultEmpty(rs, "server_id")); Line 50: glusterServer.setGlusterServerUuid(getGuidDefaultEmpty(rs, "gluster_server_uuid")); Line 51: String knownAddresses = rs.getString("known_addresses"); Line 52: if (!StringUtils.isBlank(knownAddresses)) { use StringUtils.isNotBlank() Line 53: String[] knownAddressArray = knownAddresses.split(","); Line 54: ArrayList<String> knownAddressList = new ArrayList<>(); Line 55: for (String addr : knownAddressArray) { Line 56: knownAddressList.add(addr); Line 50: glusterServer.setGlusterServerUuid(getGuidDefaultEmpty(rs, "gluster_server_uuid")); Line 51: String knownAddresses = rs.getString("known_addresses"); Line 52: if (!StringUtils.isBlank(knownAddresses)) { Line 53: String[] knownAddressArray = knownAddresses.split(","); Line 54: ArrayList<String> knownAddressList = new ArrayList<>(); can type be reduced to List ? Line 55: for (String addr : knownAddressArray) { Line 56: knownAddressList.add(addr); Line 57: } Line 58: glusterServer.setKnownAddresses(knownAddressList); Line 76: protected RowMapper<GlusterServer> createEntityRowMapper() { Line 77: return glusterServerRowMapper; Line 78: } Line 79: Line 80: @Override please add unit-tests for the new stored procedures Line 81: public void addKnownAddress(Guid serverId, String address) { Line 82: getCallsHandler().executeModification("AddGlusterServerKnownAddress", Line 83: getCustomMapSqlParameterSource().addValue("server_id", serverId) Line 84: .addValue("known_address", address)); -- To view, visit https://gerrit.ovirt.org/38149 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8fa407d6a525e73b89a79d063517798283b520fd Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.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
