Moti Asayag has posted comments on this change. Change subject: engine: refactor CollectVdsNetworkDataVDSCommand ......................................................................
Patch Set 2: Code-Review+1 (12 comments) very nice patch! gave mostly picky comments. could be addressed either in this patch or by a separate one, since this in only refactor. http://gerrit.ovirt.org/#/c/33905/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersister.java: Line 8: Line 9: public interface HostNetworkTopologyPersister { Line 10: Line 11: /** Line 12: * Persist this VDS network topology to DB. Set this host to non-operational in case networks doesn't comply with s/VDS/host Line 13: * cluster rules: Line 14: * <ul> Line 15: * <li>All mandatory networks(optional=false) should be implemented by the host. Line 16: * <li>All VM networks must be implemented with bridges. http://gerrit.ovirt.org/#/c/33905/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java: Line 40: } Line 41: Line 42: // Don't new me - use getInstance method Line 43: private HostNetworkTopologyPersisterImpl() { Line 44: super(); why a call to super is required ? Line 45: } Line 46: Line 47: @Override Line 48: public NonOperationalReason persistAndEnforceNetworkCompliance(VDS host, Line 54: Line 55: return enforceNetworkCompliance(host, skipManagementNetwork, dbIfaces); Line 56: } Line 57: Line 58: private NonOperationalReason enforceNetworkCompliance(VDS host, i'd claim the compliance enforcement logic could/should be extracted into its own class. but since this is a plain refactor - let's do it incrementally and not handle it ATM. Line 59: boolean skipManagementNetwork, Line 60: List<VdsNetworkInterface> dbIfaces) { Line 61: if (host.getStatus() != VDSStatus.Maintenance) { Line 62: Line 91: final Map<String, Network> clusterNetworksByName = Entities.entitiesByName(clusterNetworks); Line 92: final Collection<Network> dbHostNetworks = findNetworksOnInterfaces(dbIfaces, clusterNetworksByName); Line 93: logChangedDisplayNetwork(host, dbHostNetworks, dbIfaces); Line 94: logUnsynchronizedNetworks(host, clusterNetworksByName); Line 95: } please add a space line after closing bracket :-) Line 96: return NonOperationalReason.NONE; Line 97: } Line 98: Line 99: @Override Line 117: } Line 118: } Line 119: } Line 120: Line 121: private void logChangedDisplayNetwork( please join with the next line and rename vds to host. Line 122: VDS vds, Line 123: Collection<Network> engineHostNetworks, Line 124: Collection<VdsNetworkInterface> engineInterfaces) { Line 125: Line 151: private boolean isVmRunningOnHost(Guid hostId) { Line 152: return !DbFacade.getInstance().getVmDynamicDao().getAllRunningForVds(hostId).isEmpty(); Line 153: } Line 154: Line 155: private Collection<Network> findNetworksOnInterfaces( please join with the next line Line 156: Collection<VdsNetworkInterface> ifaces, Line 157: Map<String, Network> clusterNetworksByName) { Line 158: final Collection<Network> networks = new ArrayList<Network>(); Line 159: for (VdsNetworkInterface iface : ifaces) { Line 174: } Line 175: if (NetworkUtils.isManagementNetwork(network)) { Line 176: managementNetwork = network; Line 177: } Line 178: } please add space line Line 179: return managementNetwork; Line 180: } Line 181: Line 182: private void logUnsynchronizedNetworks(VDS vds, Map<String, Network> networks) { Line 178: } Line 179: return managementNetwork; Line 180: } Line 181: Line 182: private void logUnsynchronizedNetworks(VDS vds, Map<String, Network> networks) { s/vds/host Line 183: List<String> networkNames = new ArrayList<String>(); Line 184: NetworkQoSDao qosDao = DbFacade.getInstance().getNetworkQosDao(); Line 185: Line 186: for (VdsNetworkInterface iface : vds.getInterfaces()) { Line 207: private void persistTopology(List<VdsNetworkInterface> reportedNics, Line 208: List<VdsNetworkInterface> dbNics, Line 209: Map<String, VdsNetworkInterface> nicsByName) { Line 210: InterfaceDao interfaceDAO = DbFacade.getInstance().getInterfaceDao(); Line 211: List<String> updatedIfaces = new ArrayList<String>(); please remove "String" type from the right side Line 212: List<VdsNetworkInterface> dbIfacesToBatch = new ArrayList<>(); Line 213: Map<String, VdsNetworkInterface> hostNicsByNames = Entities.entitiesByName(reportedNics); Line 214: Line 215: // First we check what interfaces need to update/delete Line 260: } Line 261: } Line 262: } Line 263: Line 264: private String getVmNetworksImplementedAsBridgeless(VDS vds, List<Network> clusterNetworks) { s/vds/host Line 265: Map<String, VdsNetworkInterface> interfacesByNetworkName = Line 266: Entities.hostInterfacesByNetworkName(vds.getInterfaces()); Line 267: List<String> networkNames = new ArrayList<String>(); Line 268: Line 276: Line 277: return StringUtils.join(networkNames, ","); Line 278: } Line 279: Line 280: private String getMissingOperationalClusterNetworks(VDS vds, List<Network> clusterNetworks) { s/vds/host Line 281: Map<String, Network> vdsNetworksByName = Entities.entitiesByName(vds.getNetworks()); Line 282: List<String> networkNames = new ArrayList<String>(); Line 283: Line 284: for (Network net : clusterNetworks) { Line 290: } Line 291: return StringUtils.join(networkNames, ","); Line 292: } Line 293: Line 294: private void setNonOperational(VDS vds, NonOperationalReason reason, Map<String, String> customLogValues) { s/vds/host Line 295: ResourceManager.getInstance() Line 296: .getEventListener() Line 297: .vdsNonOperational(vds.getId(), reason, true, Guid.Empty, customLogValues); Line 298: } -- To view, visit http://gerrit.ovirt.org/33905 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I798abe6597c06df873ac94f61981ff1dccc07bc8 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
