Alona Kaplan 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 NetworkInterface). Either way it is not related to this patch:) 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? If you want to make the class testable you can add getInterfaceDao() public method to the class. What are the benefits of passing it a parameter to the ctor? 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 of the method is not accurate. 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. 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 class for the enforcer. 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 to persist the topology. Those methods should be private. The persister should have only one public method- persistTopology, this is the only method should be called from here. 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 <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
