Alona Kaplan has posted comments on this change. Change subject: core: refactor of HostNetworkAttachmentPersister ......................................................................
Patch Set 36: (4 comments) https://gerrit.ovirt.org/#/c/36141/36/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 111: //TODO MM: what should happen if there are more attachments referencing same network? Should it insert multiple records to db? Or should it update to last version? Or fail? Line 112: //TODO MM: what if attachment has nicID already assigned? Line 113: for (NetworkAttachment attachment : userNetworkAttachments) { Line 114: boolean userAttachmentReferencesNetworkBoundToNic = networkConfiguredOnHost(attachment.getNetworkId()); Line 115: if (userAttachmentReferencesNetworkBoundToNic) { why not just- if (networkConfiguredOnHost(attachment.getNetworkId())) as it was previously, I think it was more readable. Line 116: VdsNetworkInterface nicToWhichIsNetworkBound = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 117: attachment.setNicId(baseInterfaceOfNic(nicToWhichIsNetworkBound).getId()); Line 118: boolean alreadyExistingAttachment = dbAttachmentsById.containsKey(attachment.getId()); Line 119: Line 113: for (NetworkAttachment attachment : userNetworkAttachments) { Line 114: boolean userAttachmentReferencesNetworkBoundToNic = networkConfiguredOnHost(attachment.getNetworkId()); Line 115: if (userAttachmentReferencesNetworkBoundToNic) { Line 116: VdsNetworkInterface nicToWhichIsNetworkBound = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 117: attachment.setNicId(baseInterfaceOfNic(nicToWhichIsNetworkBound).getId()); Why do you need this set, isn't the attachment already contain the correct value. Maybe you should avoid adding the attachment to the db in case the nicId on the attachment doesn't match the baseInterfaceOfNic(reportedNicToWhichIsNetworkBound).getId() Line 118: boolean alreadyExistingAttachment = dbAttachmentsById.containsKey(attachment.getId()); Line 119: Line 120: if (alreadyExistingAttachment) { Line 121: networkAttachmentDao.update(attachment); Line 128: } Line 129: } Line 130: Line 131: private VdsNetworkInterface baseInterfaceOfNic(VdsNetworkInterface nic) { Line 132: return nic.getBaseInterface() == null ? nic : nicsByName.get(nic.getBaseInterface()); You can change the method with the following- return nicsByName.get(NetworkUtils.stripVlan(nic)); Line 133: } Line 134: Line 135: /** Line 136: * Removes {@link org.ovirt.engine.core.common.businessentities.network.NetworkAttachment NetworkAttachments} Line 148: } Line 149: Line 150: /** Line 151: * @param networkId network ID. Line 152: * @return true if given network ID describes network bound to any given NIC. to any given reported NIC. Line 153: */ Line 154: private boolean networkConfiguredOnHost(Guid networkId) { Line 155: return reportedNetworksById.containsKey(networkId); Line 156: } -- To view, visit https://gerrit.ovirt.org/36141 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibecf02fe47f42c2184c1f010f794592944c55e62 Gerrit-PatchSet: 36 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
