Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 35: (5 comments) https://gerrit.ovirt.org/#/c/33333/35/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 91: } Line 92: Line 93: @Override Line 94: protected boolean canDoAction() { Line 95: qosDao = getDbFacade().getHostNetworkQosDao(); > The use of DbFacade should be avoided. You can inject the HostNetworkQosDao Done Line 96: Line 97: VDS host = getVds(); Line 98: Line 99: NetworkAttachmentCompleter completer = new NetworkAttachmentCompleter(getExistingNics()); Line 105: getExistingNics(), Line 106: getExistingAttachments(), Line 107: getClusterNetworks(), Line 108: managementNetworkUtil Line 109: ); > Why did you move it to a new line? what are you talking about? I'm doing gazillions actions simultaneously, I do not remember what I did and where. Also some actions can be done by auto-formatter. Are you talking about ');' being on niew line or what? Line 110: Line 111: getReturnValue().getCanDoActionMessages().addAll(validator.validate()); Line 112: getReturnValue().getCanDoActionMessages().addAll(new HostValidator(host, isInternalExecution()).validate()); Line 113: Line 108: managementNetworkUtil Line 109: ); Line 110: Line 111: getReturnValue().getCanDoActionMessages().addAll(validator.validate()); Line 112: getReturnValue().getCanDoActionMessages().addAll(new HostValidator(host, isInternalExecution()).validate()); > The HostValidator validations are very basic (at least the host existence o no, validators will fails if completer did not run before them. It was designed like that. Line 113: Line 114: boolean noErrorMessageRecorded = getReturnValue().getCanDoActionMessages().isEmpty(); Line 115: return noErrorMessageRecorded; Line 116: } Line 204: Line 205: private List<VdsNetworkInterface> getRemovedBonds() { Line 206: if (removedBonds == null) { Line 207: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 208: this.removedBonds = Entities.entitiesById(removedBondIds, getExistingNics()); > The 'this' is redundant. Done Line 209: } Line 210: Line 211: return removedBonds; Line 212: } Line 289: VdsNetworkInterface iface = nics.get(attachment.getNicId()); Line 290: boolean qosConfiguredOnInterface = NetworkUtils.qosConfiguredOnInterface(iface, network); Line 291: networkToConfigure.setQosConfiguredOnInterface(qosConfiguredOnInterface); Line 292: if (qosConfiguredOnInterface) { Line 293: networkToConfigure.setRelevantQos( > Why relvantQos and not just networkToConfigure.setQos(..)? Done Line 294: iface.isQosOverridden() ? iface.getQos() : qosDao.get(network.getQosId())); Line 295: } Line 296: Line 297: networksToConfigure.add(networkToConfigure); -- 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: 35 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: 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
