Alona Kaplan has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (11 comments) Didn't finish reviewing the patch yet. Publishing comments up to now. https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java: Line 64: private static final Logger log = LoggerFactory.getLogger(HostSetupNetworksCommand.class); Line 65: private Map<Guid, Network> clusterNetworks; Line 66: private Set<String> removedNetworks; Line 67: private Set<String> removedBondNames; Line 68: private List<VdsNetworkInterface> existingBondVdsInterfacesToBeRemoved; why not just- removedBonds? Line 69: private List<VdsNetworkInterface> existingNics; Line 70: private List<NetworkAttachment> existingAttachments; Line 71: private List<HostNetwork> networksToConfigure; Line 72: Line 160: runVdsCommand(VDSCommandType.GetCapabilities, Line 161: new VdsIdAndVdsVDSCommandParametersBase(getVds())); Line 162: VDS updatedHost = (VDS) returnValue.getReturnValue(); Line 163: persistNetworkChanges(updatedHost); Line 164: } Please add logMonitorLockReleased("Host setup networks"); Line 165: Line 166: setSucceeded(true); Line 167: } Line 168: } Line 220: && getParameters().getBonds().isEmpty() Line 221: && getRemovedBondNames().isEmpty(); Line 222: } Line 223: Line 224: private List<VdsNetworkInterface> getExistingBondVdsInterfaceToBeRemoved() { why not just- getRemovedBonds? Line 225: if (existingBondVdsInterfacesToBeRemoved == null) { Line 226: Line 227: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 228: int removedBondsSize = removedBondIds.size(); Line 227: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 228: int removedBondsSize = removedBondIds.size(); Line 229: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondsSize); Line 230: Line 231: if (!Entities.entitiesById(removedBondIds, existingNics, removedBondInterfaces)) { please call getExistingNics() instead of directly passing existingNics. Line 232: throw new IllegalArgumentException("Unable to find requested bond"); Line 233: } else { Line 234: this.existingBondVdsInterfacesToBeRemoved = removedBondInterfaces; Line 235: } Line 228: int removedBondsSize = removedBondIds.size(); Line 229: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondsSize); Line 230: Line 231: if (!Entities.entitiesById(removedBondIds, existingNics, removedBondInterfaces)) { Line 232: throw new IllegalArgumentException("Unable to find requested bond"); It passed the canDo, so IIUC, there is no way to get this exception. I suggest Entities.entitiesById to return a map of id to entity (if the id doesn't have existing entity it won't be added to the map). It can also help you in - HostSetupNetworksValidator.validateAndInitRemovedBondInterfaces(..)- please see a related commet there. Since getExistingBondVdsInterfaceToBeRemoved() is called after the can do action, we can be sure that there are no non-exixting ids. So Entities.entitiesById(removedBondIds, existingNics).values() can be used. Line 233: } else { Line 234: this.existingBondVdsInterfacesToBeRemoved = removedBondInterfaces; Line 235: } Line 236: } Line 278: return existingAttachments; Line 279: } Line 280: Line 281: private Set<String> getRemovedNetworks() { Line 282: if (removedNetworks == null) { Same comment as for getExistingBondVdsInterfaceToBeRemoved() Line 283: Set<Guid> removedNetworkAttachmentIds = getParameters().getRemovedNetworkAttachments(); Line 284: int removedNetworkAttachmentIdsSize = removedNetworkAttachmentIds.size(); Line 285: removedNetworks = new HashSet<>(removedNetworkAttachmentIdsSize); Line 286: https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 109: public boolean validateAndInitRemovedBondInterfaces() { Line 110: Set<Guid> removedBondIds = params.getRemovedBonds(); Line 111: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondIds.size()); Line 112: if (!Entities.entitiesById(removedBondIds, existingIfaces.values(), removedBondInterfaces)) { Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); The bond id should be specified in the message. Maybe entitiedById can return a map of id to entity. If there is invalid idit won't be aaded to the map. Each removedBondId that is not in the result map will have a validation. Line 114: return false; Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java: Line 206: * Line 207: * @param requestedIds id to find. Line 208: * @param existingEntities all existing entities Line 209: * @param resultCollection result collection Line 210: * @param <E> Why does it part of the java doc? Line 211: * @param <I> Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 207: * @param requestedIds id to find. Line 208: * @param existingEntities all existing entities Line 209: * @param resultCollection result collection Line 210: * @param <E> Line 211: * @param <I> same Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 215: */ Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 215: */ Line 216: public static <E extends BusinessEntity<I>, I extends Serializable> boolean entitiesById(Collection<I> requestedIds, As I wrote in previous comments, I think this method should be changed. Since this class has a method called- entitiesByName which returns a name to entity map. I would except this new method which is called entitiesById to return id to entity map (id's with non-existing entities won't be added to the map). As I mentioned before, it will make this method more useful since you will easily know what are the problematic ids (the id are not contained in the result map). Line 217: Collection<E> existingEntities, Line 218: Collection<E> resultCollection) { Line 219: Line 220: if (!resultCollection.isEmpty()) { Line 223: Line 224: Map<I, E> existingEntitiesMap = businessEntitiesById(existingEntities); Line 225: for (I requestedId : requestedIds) { Line 226: if (requestedId == null) { Line 227: log.warn("Request to find entity by null valued ID; ignoring."); It is a utility method, it shouldn't contain logs. Maybe the user of the method is ok with having non-existing ids. Line 228: continue; Line 229: } Line 230: Line 231: if (existingEntitiesMap.containsKey(requestedId)) { -- To view, visit https://gerrit.ovirt.org/33333 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I60077019f308f371f21fb7b5545ba48adb38bd06 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@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
