Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 35: (11 comments) https://gerrit.ovirt.org/#/c/33333/35/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 221: candidateAttachments.addAll(newAttachments); Line 222: return candidateAttachments; Line 223: } Line 224: Line 225: private boolean validNewOrModifiedBonds() { > The final version of this method has a comment- that each violation should I don't understand what you're saying by that. Line 226: boolean passed = true; Line 227: for (Bond modifiedBond : params.getBonds()) { Line 228: if (modifiedBond.getName() == null) { Line 229: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 283: Line 284: /** Line 285: * @return true if there's network attachment related to given nic, which gets removed upon request. Line 286: */ Line 287: private boolean isNetworkAttachmentRemoved(VdsNetworkInterface nic) { > I am assuming you will copy to this patch the final revision of validNewOrM this is getting a little (a lot) crazy. Really, I cannot track content and dependencies of all patches. It's almost undoable with all rebases&conflicts, I cannot handle relations among different patch versions. I don't know if, when or where this becomes unused. It's used here, it cannot be removed here. So this change does not relate to this patch. Yes it's unused in the end. Lets fix this problem where it becomes unused. Line 288: for (NetworkAttachment attachment : removedNetworkAttachments) { Line 289: if (Objects.equals(nic.getName(), attachment.getNicName())) { Line 290: return true; Line 291: } Line 301: * @param bondName name of bond we're examining Line 302: * Line 303: * @return true if bond specified by name is present in request and does not contain given slave. Line 304: */ Line 305: private boolean bondIsUpdatedAndDoesNotContainCertainSlave(String slaveName, String bondName) { > Maybe isSlaveRemovedFromBond this not it. We don't know if it's removed. We just know it's not there. That's a different thing. Line 306: for (Bond bond : params.getBonds()) { Line 307: boolean isRequiredBond = bond.getName().equals(bondName); Line 308: if (isRequiredBond) { Line 309: boolean updatedBondDoesNotContainGivenSlave = !bond.getSlaves().contains(slaveName); Line 303: * @return true if bond specified by name is present in request and does not contain given slave. Line 304: */ Line 305: private boolean bondIsUpdatedAndDoesNotContainCertainSlave(String slaveName, String bondName) { Line 306: for (Bond bond : params.getBonds()) { Line 307: boolean isRequiredBond = bond.getName().equals(bondName); > Why aren't you using the bondsMap? which bonds map? I can't see any. Or are you suggesting creating one just for this method? (pointless, asymptotically equal, when exactly measured it would be worse). Line 308: if (isRequiredBond) { Line 309: boolean updatedBondDoesNotContainGivenSlave = !bond.getSlaves().contains(slaveName); Line 310: return updatedBondDoesNotContainGivenSlave; Line 311: } Line 328: boolean passed = true; Line 329: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 330: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 331: Line 332: if (!validate(validator.networkAttachmentIsSet())) { > validator.networkAttachmentIsSet() checks if the attachment is null. In thi ok. So both ways are fine. With or without check. The last state is with check. Lets not waste time on stuff which does not matter.Done. Line 333: passed = false; Line 334: continue; Line 335: } Line 336: Line 351: Line 352: return passed; Line 353: } Line 354: Line 355: private ValidationResult referencedNetworkAttachmentActuallyExists(Guid networkAttachmentId) { > maybe- modifiedAttachmentExists please keep in mind, that each rename generate MANY conflicts during rebase. Renaming of sole method is not cheap in our case, it can costs even 20 minutes. So all 'maybe's should be reconsidered twice. Renamed. Done. Line 356: boolean doesNotReferenceExistingNetworkAttachment = networkAttachmentId == null; Line 357: if (doesNotReferenceExistingNetworkAttachment) { Line 358: return ValidationResult.VALID; Line 359: } Line 352: return passed; Line 353: } Line 354: Line 355: private ValidationResult referencedNetworkAttachmentActuallyExists(Guid networkAttachmentId) { Line 356: boolean doesNotReferenceExistingNetworkAttachment = networkAttachmentId == null; > maybe- isNewAttachment no, that's not the same meaning. If I reference something which is not existent, it does not mean, that it's a new item. Line 357: if (doesNotReferenceExistingNetworkAttachment) { Line 358: return ValidationResult.VALID; Line 359: } Line 360: Line 365: } Line 366: Line 367: return new ValidationResult(VdcBllMessages.NETWORK_ATTACHMENT_NOT_EXISTS); Line 368: } Line 369: > Please remove the white space. Done Line 370: private boolean validRemovedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { Line 371: List<Guid> invalidIds = Entities.idsNotReferencingExistingRecords(params.getRemovedNetworkAttachments(), Line 372: existingIfacesById); Line 373: if (!invalidIds.isEmpty()) { Line 371: List<Guid> invalidIds = Entities.idsNotReferencingExistingRecords(params.getRemovedNetworkAttachments(), Line 372: existingIfacesById); Line 373: if (!invalidIds.isEmpty()) { Line 374: for (Guid problematicId : invalidIds) { Line 375: addViolation(VdcBllMessages.NETWORK_ATTACHMENT_NOT_EXISTS, problematicId.toString()); > Refer to the comment in line 339. yes, we'll have to verify all VdcBllMessages referenced. If we even support multiple failing entities altogether. I'll fix this once error handling is resolved. Marking it as done now. Done. Line 376: Line 377: } Line 378: return false; Line 379: } Line 378: return false; Line 379: } Line 380: Line 381: boolean passed = true; Line 382: for (NetworkAttachment attachment : this.removedNetworkAttachments) { > the "this" is redundant. Done Line 383: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 384: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 385: if (!(validate(validator.networkAttachmentIsSet(), attachment.getId().toString()) Line 386: && validate(validator.notExternalNetwork()) Line 381: boolean passed = true; Line 382: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 383: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 384: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 385: if (!(validate(validator.networkAttachmentIsSet(), attachment.getId().toString()) > I agree with you, (as you answered on patchset 26) this validation should b Done Line 386: && validate(validator.notExternalNetwork()) Line 387: && validate(validator.notManagementNetwork()) Line 388: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 389: passed = false; -- 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
