Martin Mucha has posted comments on this change. Change subject: engine: Introduce network attachments command ......................................................................
Patch Set 21: (9 comments) https://gerrit.ovirt.org/#/c/34971/21/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java: Line 38: @Override Line 39: protected boolean canDoAction() { Line 40: VDS host = getVds(); Line 41: Line 42: if (host == null) { > I guess you have the same check for update and remove. Why isn't part of a Done Line 43: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 44: } Line 45: Line 46: if (!HostSetupNetworksCommand.SUPPORTED_HOST_STATUSES.contains(host.getStatus())) { Line 42: if (host == null) { Line 43: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 44: } Line 45: Line 46: if (!HostSetupNetworksCommand.SUPPORTED_HOST_STATUSES.contains(host.getStatus())) { > same Done Line 47: addCanDoActionMessage(VdcBllMessages.VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL); Line 48: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL); Line 49: } Line 50: Line 50: Line 51: NetworkAttachmentCompleter completer = new NetworkAttachmentCompleter(getHostInterfaces()); Line 52: completer.completeNicName(getParameters().getNetworkAttachment()); Line 53: Line 54: return validateNetworkAttachment() && validateHostInterface() && validateCrossAttachments(); > I don't think you need any of these extra validations. Actually, I don't th Done Line 55: } Line 56: Line 57: @Override Line 58: protected void executeCommand() { Line 70: String configuredNicName = getParameters().getNetworkAttachment().getNicName(); Line 71: VdsNetworkInterface configuredNic = Entities.entitiesByName(getHostInterfaces()).get(configuredNicName); Line 72: List<NetworkAttachment> attachmentsOnNic = Line 73: getDbFacade().getNetworkAttachmentDao().getAllForNic(configuredNic.getId()); Line 74: for (NetworkAttachment attachment : attachmentsOnNic) { > You can use- Linq.firstOrNul(..), I think it is more readable. Done Line 75: if (attachment.getNetworkId().equals(getParameters().getNetworkAttachment().getNetworkId())) { Line 76: createdAttachmentId = attachment.getId(); Line 77: } Line 78: } https://gerrit.ovirt.org/#/c/34971/21/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java: Line 27: } Line 28: Line 29: @Override Line 30: protected boolean canDoAction() { Line 31: VDS host = getVds(); > Please see the canDoAction related comment in- AddNetworkAttachmentCommand. Done Line 32: Line 33: if (host == null) { Line 34: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 35: } Line 52: protected void executeCommand() { Line 53: HostSetupNetworksParameters params = new HostSetupNetworksParameters(getParameters().getVdsId()); Line 54: NetworkAttachment networkAttachmentForRemove = Line 55: getDbFacade().getNetworkAttachmentDao().get(getParameters().getNetworkAttachment().getId()); Line 56: params.getRemovedNetworkAttachments().add(networkAttachmentForRemove.getId()); > You're querying the networkAttachment by id from the db just to get the id? I'm not sure why it's there. It can be there to verify actual record existence (to verify if user do not try to remove not existing record, which should be reported as invalid request). but we can remove it, since I'm covering that in rest layer lot of patches later. But what's really weird here is passing VdsId from frontend. Am I right? NetworkAttachment cannot be reused for multiple hosts (vds-es), so here we're expecting user to send 2 valid, coherent, IDs. NetworkAttachments and hosts. I think this should be removed, vds id obtained from the db and parameter class changed to just IdParameters. What do you think? Line 57: VdcReturnValueBase returnValue = runInternalAction(VdcActionType.HostSetupNetworks, params); Line 58: propagateFailure(returnValue); Line 59: setSucceeded(returnValue.getSucceeded()); Line 60: } https://gerrit.ovirt.org/#/c/34971/21/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java: Line 36: addValidationGroup(UpdateEntity.class); Line 37: } Line 38: Line 39: @Override Line 40: protected boolean canDoAction() { > Please see the canDoAction related comment in- AddNetworkAttachmentCommand. Done Line 41: VDS host = getVds(); Line 42: Line 43: if (host == null) { Line 44: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 54: && validateHostInterface() Line 55: && validateCrossAttachments(); Line 56: } Line 57: Line 58: private NetworkAttachmentCompleter getNetworkAttachmentCompleter() { > Why do you need this getter? Do you use the completer anywhere but the canD once all validations are gone, it's not needed at all.Done Line 59: if (completer == null) { Line 60: completer = new NetworkAttachmentCompleter(getHostInterfaces()); Line 61: } Line 62: Line 63: return completer; Line 64: } Line 65: Line 66: @Override Line 67: protected void executeCommand() { > Since add and update has the same api to the HostSetupNetworks command (and Done Line 68: HostSetupNetworksParameters params = new HostSetupNetworksParameters(getParameters().getVdsId()); Line 69: params.getNetworkAttachments().add(getParameters().getNetworkAttachment()); Line 70: VdcReturnValueBase returnValue = runInternalAction(VdcActionType.HostSetupNetworks, params); Line 71: propagateFailure(returnValue); -- To view, visit https://gerrit.ovirt.org/34971 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c1b7a4e0b5ceacf6ea436c4ede84e414817dbdd Gerrit-PatchSet: 21 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
