Moti Asayag has posted comments on this change. Change subject: engine: Introduce HostNetworkInterfacesPersister ......................................................................
Patch Set 7: (6 comments) http://gerrit.ovirt.org/#/c/34070/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/VdsNetworkInterface.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/VdsNetworkInterface.java: Line 16: import org.ovirt.engine.core.compat.Guid; Line 17: Line 18: /** Line 19: * <code>VdsNetworkInterface</code> defines a type of {@link BaseNetworkInterface} for instances of Line 20: * {@link org.ovirt.engine.core.common.businessentities.VDS}. > I think the problem in this doc was BaseNetworkInterface (should be Network Done. no, the problem is with {@link className} where the className is without the full package name. It causes eclipse to automatically add the import. But checkstyle doesn't identify ant "real" usage of that class, hence complains about unused import. When the it is provided as {@link full-qualified-class-name} - eclipse doesn't bother to add that import, hence everyone happy (eclipse, checkstyle, me...) I'll send a separate patch for that. Line 21: */ Line 22: @ValidNetworkConfiguration Line 23: public class VdsNetworkInterface extends NetworkInterface<VdsNetworkStatistics> { Line 24: private static final long serialVersionUID = -6347816237220936283L; http://gerrit.ovirt.org/#/c/34070/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java: Line 17: private final List<VdsNetworkInterface> dbNics; Line 18: private List<VdsNetworkInterface> nicsForUpdate; Line 19: private final Map<String, VdsNetworkInterface> userConfiguredNicsByName; Line 20: Line 21: public HostNetworkInterfacesPersisterImpl(InterfaceDao interfaceDao, > Why should the intrerfaceDao be a parameter of the ctor? Few reasons not to introduce getInterfaceDao(): 1. I don't need it elsewhere outside of the test. It is not part of the class declared interface. 2. For testing that class with the getInterfaceDao(), I'd have to mock the method via "spy"ing the instance - which i'd like to refrain. I could test it explicitly and not a mocked instance of it. Line 22: List<VdsNetworkInterface> reportedNics, Line 23: List<VdsNetworkInterface> dbNics, Line 24: List<VdsNetworkInterface> userConfiguredNics) { Line 25: this.interfaceDao = interfaceDao; Line 55: interfaceDao.saveStatisticsForVds(nicForCreate.getStatistics()); Line 56: } Line 57: } Line 58: Line 59: private List<VdsNetworkInterface> extractNicsForCreate() { > Since this method also overrides the nic with user configuration the name o ok - i'll rename to "prepareNicsForCreate" Line 60: List<VdsNetworkInterface> nicsForCreate = new ArrayList<>(); Line 61: Set<String> nicsNamesForUpdate = Entities.objectNames(getNicsForUpdate()); Line 62: for (VdsNetworkInterface reportedNic : reportedNics) { Line 63: if (!nicsNamesForUpdate.contains(reportedNic.getName())) { Line 68: Line 69: return nicsForCreate; Line 70: } Line 71: Line 72: private List<VdsNetworkInterface> extractNicsForUpdate() { > Same here- the name of the method is not accurate. ok - i'll rename to "prepareNicsForUpdate" Line 73: List<VdsNetworkInterface> nicsForUpdate = new ArrayList<>(); Line 74: Line 75: for (VdsNetworkInterface dbNic : dbNics) { Line 76: if (reportedNicsByNames.containsKey(dbNic.getName())) { http://gerrit.ovirt.org/#/c/34070/7/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 53: enforceNetworkCompliance > Now you have a separate class for the persister. Consider adding a separate absolutely - i gave the same comment to Yevgeny when he introduced that class. It is just not part of this patch nor doesn't required by me for the network-attachment effort. So it will have to wait unless some else wishes to take over it. Line 225: new HostNetworkInterfacesPersisterImpl(DbFacade.getInstance().getInterfaceDao(), Line 226: reportedNics, Line 227: dbNics, Line 228: userConfiguredNics); Line 229: > I think the topology class shouldn't be aware to the inner actions that are Done Line 230: networkInterfacesPersister.removeUnreportedInterfaces(); Line 231: networkInterfacesPersister.updateModifiedInterfaces(); Line 232: networkInterfacesPersister.createNewInterfaces(); Line 233: } -- To view, visit http://gerrit.ovirt.org/34070 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec0701a302781a9fa147c65818764bddf8429828 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
