Alona Kaplan has posted comments on this change. Change subject: core: refactored gathering violations to separate class ......................................................................
Patch Set 47: (15 comments) 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 10: Line 11: import org.apache.commons.lang.StringUtils; Line 12: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 13: Line 14: public class FailingValidationResults<T> { 1. Why do you need generic here? Isn't the violatingEntity always a String? 2. I would call the class- ValitationsWithViolatingEntities. Line 15: protected static final String VIOLATING_ENTITIES_LIST_FORMAT = "${0}_LIST {1}"; Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); Line 14: public class FailingValidationResults<T> { Line 15: protected static final String VIOLATING_ENTITIES_LIST_FORMAT = "${0}_LIST {1}"; Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); maybe- validationResultToViolatingEntity (why do you need to specify it is a map in the parameter name?) Line 19: Line 20: public Collection<ValidationResult> failingValidationResults() { Line 21: return validationResultToViolationCauseMap.keySet(); Line 22: } Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); Line 19: Line 20: public Collection<ValidationResult> failingValidationResults() { A method should start with a verb- getFailingValidationResults() Line 21: return validationResultToViolationCauseMap.keySet(); Line 22: } Line 23: Line 24: /** Line 32: if (validationResult.isValid()) { Line 33: return false; Line 34: } Line 35: Line 36: List<T> violationCauses = getCausesList(validationResult); You can use the MultiValueMapUtils.addToMap(..) instead. Line 37: Line 38: if (violatingEntity != null) { Line 39: violationCauses.add(violatingEntity); Line 40: } Line 40: } Line 41: return true; Line 42: } Line 43: Line 44: //TODO MM: use helper. Please move it to your TODO list. The code cannot be merged with this unclear TODO (what helper?). Line 45: private List<T> getCausesList(ValidationResult validationResult) { Line 46: List<T> violationCauses = validationResultToViolationCauseMap.get(validationResult); Line 47: if (violationCauses == null) { Line 48: violationCauses = new ArrayList<>(); Line 41: return true; Line 42: } Line 43: Line 44: //TODO MM: use helper. Line 45: private List<T> getCausesList(ValidationResult validationResult) { Maybe- getViolatingEntities Line 46: List<T> violationCauses = validationResultToViolationCauseMap.get(validationResult); Line 47: if (violationCauses == null) { Line 48: violationCauses = new ArrayList<>(); Line 49: validationResultToViolationCauseMap.put(validationResult, violationCauses); Line 55: return addViolation(validationResult, null); Line 56: } Line 57: Line 58: public boolean addViolation(VdcBllMessages vdcBllMessage) { Line 59: return addViolation(new ValidationResult(vdcBllMessage), null); Please call- addViolation(vdcBllMessage, null) Line 60: } Line 61: Line 62: public boolean addViolation(VdcBllMessages vdcBllMessage, T violatingEntity) { Line 63: return addViolation(new ValidationResult(vdcBllMessage), violatingEntity); 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. 2. Maybe getMessageToViolatingEntities(..) 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 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 81: List<T> causes = result.get(vdcBllMessage); You can use the MultiValueMapUtils.addToMap(..) instead. Line 82: if (causes == null) { Line 83: causes = new ArrayList<>(); Line 84: result.put(vdcBllMessage, causes); Line 85: } Line 98: String messageName = vdcBllMessage.name(); Line 99: Line 100: violationMessages.add(messageName); Line 101: Line 102: String causesString = StringUtils.join(entry.getValue(), ", "); Please replace causes by violatingEntities (here and in the rest of the class). It is not the causes, it is the list of the problematic entities. Line 103: String formattedCausesString = Line 104: MessageFormat.format(VIOLATING_ENTITIES_LIST_FORMAT, messageName, causesString); Line 105: violationMessages.add(formattedCausesString); Line 106: } https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java: Line 34: } Line 35: Line 36: @Override Line 37: protected boolean canDoAction() { Line 38: // VDS host = getVds(); Please remove the commented code and consequently, remove this class from the patch. Line 39: // Line 40: // FailingValidationResults<String> validationResults = new HostValidator(host, isInternalExecution()).validate(); Line 41: // if (!validationResults.isValid()) { Line 42: // getReturnValue().getCanDoActionMessages().addAll(validationResults.translateToViolationMessages()); https://gerrit.ovirt.org/#/c/37154/47/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 95: protected boolean canDoAction() { Line 96: qosDao = getDbFacade().getHostNetworkQosDao(); Line 97: Line 98: VDS host = getVds(); Line 99: You should execute HostValidator.hostExists(..) here (before the completer), as I mentioned in a previous patch. But it shouldn't be done in this patch (should be done in the previous one). In this patch you moved the execution of the hostValidator to the HostSetupNetworkValidator. It is wrong since at least the hostExists valdation should be executed before the completer and the initialization of the HostSetupNetworkValidator. Line 100: NetworkAttachmentCompleter completer = new NetworkAttachmentCompleter(getExistingNics()); Line 101: completer.completeNicNames(getParameters().getNetworkAttachments()); Line 102: completer.completeBondNames(getParameters().getBonds()); Line 103: 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 100: && validateNotRemovingUsedNetworkByVms()) { Line 101: Line 102: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 103: boolean valid = networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 104: && !violations.addViolations(new HostValidator(host, internalExecution).validate()) As I mention in the HostSetupNetworksCommand- the hostExist() validation is very basic and should be called before the initialization of this validator. Line 105: && violations.addViolation(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 106: && validateMtu(attachmentsToConfigure) Line 107: && validateCustomProperties(); Line 108: https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java: Line 26: @Override Line 27: protected boolean canDoAction() { Line 28: // VDS host = getVds(); Line 29: // Line 30: // FailingValidationResults<String> validationResults = new HostValidator(host, isInternalExecution()).validate(); Please remove the commented code and this class from the patch. Line 31: // if (!validationResults.isValid()) { Line 32: // getReturnValue().getCanDoActionMessages().addAll(validationResults.translateToViolationMessages()); Line 33: // return false; Line 34: // } https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java: Line 30: } Line 31: Line 32: @Override Line 33: protected boolean canDoAction() { Line 34: // VDS host = getVds(); same- please remove the commented code and this class from the patch. Line 35: // Line 36: // FailingValidationResults<String> validationResults = new HostValidator(host, isInternalExecution()).validate(); Line 37: // if (!validationResults.isValid()) { Line 38: // getReturnValue().getCanDoActionMessages().addAll(validationResults.translateToViolationMessages()); -- 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
