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 <[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
