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

Reply via email to