Alona Kaplan has posted comments on this change. Change subject: core: refactored gathering violations to separate class ......................................................................
Patch Set 47: (6 comments) This patch enables dynamically constructing the variable replacement- {message_name}_LIST instead of doing- List<String> vialotingEntities = new ArrayList<>(); .. build the vialotingEntities lists new ValidationResult(VdcBllMessages.WHATEVER_MESSAGE, MessageFormat.format("${0}_LIST {1}", VdcBllMessages.WHATEVER_MESSAGE.name(), causesString); you can build the vialotingEntities in the following way - violations.addViolation(VdcBllMessagesWHATEVER_MESSAGE, vialationEntity); I think the code for introducing this new behavior it really complicated and breaks the way commands using validators. Currently, commands calling validator using- 'validate(ValidationResult)' for example- validate(validator.smallValidation() Since your new validators have the validate method which aggregates all the small validations. I suggest- 1. The return value of the validator.validatate() method will be- 'Set<ValidationResult>'. 2. The CommandBase will have a new method- validate(Set<ValidationResult>)- it will just call the original validate(ValidationResult) for each ValidationResult in the list. 3. The FailingValidationResults (which should be renamed to ValitationsWithViolatingEntities) will have a method (instead of the translate) that iterates over all the ValidationResults in the map and adds the 'formattedCauseString' (which should be called- 'formattedVioulatingEntitiesString') to the variableReplacments of the ValidationResult. The return value of this method will be 'Set<ValidationResult>'. 4. The fact that 'violations.addViolation(..)' returns the opposite of what validate returns is very confusing. I would add to FailingValidationResults class- public 'validate(..)' methods (same as the methods you removed from HostSetupNetworValidator) and make the 'addValidation(..)' methods private. What do you think? https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FailingValidationResults.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FailingValidationResults.java: Line 72: Line 73: return !failingValidationResults.validationResultToViolationCauseMap.isEmpty(); Line 74: } Line 75: Line 76: public Map<VdcBllMessages, List<T>> getMessageToCausesMap() { > 1. this method can be private. Also, the translation to a map with the message as a key is really bug bug prone. The ValidationResult has two fields- the message and the variableReplacements (both of them are part of its equals and hashcode). When translating a map with ValidationResult as its key to a map with VdcBllMessages, you can end up with a map smaller than the original one (since ValidationResults with the same message but different variableReplacements will be merged). Line 77: Map<VdcBllMessages, List<T>> result = new HashMap<>(validationResultToViolationCauseMap.size()); Line 78: for (Map.Entry<ValidationResult, List<T>> entry : validationResultToViolationCauseMap.entrySet()) { Line 79: ValidationResult validationResult = entry.getKey(); Line 80: VdcBllMessages vdcBllMessage = validationResult.getMessage(); Line 89: return result; Line 90: Line 91: } Line 92: Line 93: public List<String> translateToViolationMessages() { I you translation you're ignoring the variableReplacements. Please look at CommandBase.validate to see how the variableReplacements should be treated. Line 94: Map<VdcBllMessages, List<T>> messageToCausesMap = getMessageToCausesMap(); Line 95: List<String> violationMessages = new ArrayList<>(messageToCausesMap.size() * 2); Line 96: for (Map.Entry<VdcBllMessages, List<T>> entry : messageToCausesMap.entrySet()) { Line 97: VdcBllMessages vdcBllMessage = entry.getKey(); https://gerrit.ovirt.org/#/c/37154/47/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 99: && validRemovedBonds() Line 100: && validateNotRemovingUsedNetworkByVms()) { Line 101: Line 102: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 103: boolean valid = networksUniquelyConfiguredOnHost(attachmentsToConfigure) The valid parameter is never used. Line 104: && !violations.addViolations(new HostValidator(host, internalExecution).validate()) Line 105: && violations.addViolation(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 106: && validateMtu(attachmentsToConfigure) Line 107: && validateCustomProperties(); Line 101: Line 102: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 103: boolean valid = networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 104: && !violations.addViolations(new HostValidator(host, internalExecution).validate()) Line 105: && violations.addViolation(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Shoudn't you add '!' (not) here. The fact that violations.addViolation returns the opposite of what validate returns is very confusing. I would add to FailingValidationResults class- public 'validate(..)' methods (same as the methods you removed from this file) and make the 'addValidation(..)' methods private. Line 106: && validateMtu(attachmentsToConfigure) Line 107: && validateCustomProperties(); Line 108: Line 109: // TODO: Cover qos change not supported and network sync. see SetupNetworkHelper.validateNetworkQos() Line 341: continue; Line 342: } Line 343: Line 344: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 345: if (!(violations.addViolation(validator.networkExists(), networkId) should be- if(validations.addViolation() || validations.addViolation() ...) Line 346: && violations.addViolation(referencedNetworkAttachmentActuallyExists(attachment.getId()), networkId) Line 347: && violations.addViolation(validator.notExternalNetwork(), networkId) Line 348: && violations.addViolation(validator.networkAttachedToCluster(), networkId) Line 349: && violations.addViolation(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 388: boolean passed = true; Line 389: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 390: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 391: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 392: if (!(violations.addViolation(validator.networkAttachmentIsSet(), attachment.getId().toString()) same here. Line 393: && violations.addViolation(validator.notExternalNetwork()) Line 394: && violations.addViolation(validator.notManagementNetwork()) Line 395: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 396: passed = false; -- To view, visit https://gerrit.ovirt.org/37154 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If92e3476ad490d5bd5957358ad69be6ace4fd4fa Gerrit-PatchSet: 47 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