Allon Mureinik has posted comments on this change. Change subject: core: Add space validation when creating snapshot ......................................................................
Patch Set 4: Code-Review-1 (13 comments) http://gerrit.ovirt.org/#/c/29258/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java: Line 30: /** A map from the ids of each domain being validated to its validator */ Line 31: private Map<Guid, StorageDomainValidator> domainValidators; Line 32: Line 33: /** A map from the domain id to its relevant disks */ Line 34: private Map<Guid, List<DiskImage>> domainsDisksMap = null; This breaks the design of the class. The class is supposed to only be stateful by it's domain, and have the disks passed as parameters to the validation (see, in analogy, StorageDomainValidator#hasSpaceForNewDisks(Collection<DiskImage>) Line 35: Line 36: /** Line 37: * Constructor from Guids Line 38: * @param sdIds A {@link Collection} of storage domain IDs to be validated Line 49: if (null == domainsDisksMap) { Line 50: return new HashMap<>(); Line 51: } Line 52: return domainsDisksMap; Line 53: } See above. Line 54: Line 55: private void setDomainsDisksMap(List<DiskImage> disksList) { Line 56: domainsDisksMap = new HashMap<>(); Line 57: for (DiskImage disk : disksList) { Line 51: } Line 52: return domainsDisksMap; Line 53: } Line 54: Line 55: private void setDomainsDisksMap(List<DiskImage> disksList) { As noted above, this shouldn't be a set method, but maybe a list2map kind of method. Line 56: domainsDisksMap = new HashMap<>(); Line 57: for (DiskImage disk : disksList) { Line 58: List<Guid> domainIds = disk.getStorageIds(); Line 59: for (Guid domainId : domainIds) { Line 61: if (null == domainDisksList) { Line 62: domainDisksList = new ArrayList<>(); Line 63: domainsDisksMap.put(domainId, domainDisksList); Line 64: } Line 65: domainDisksList.add(disk); Please use MultiValueMapUtils Line 66: } Line 67: } Line 68: } Line 69: Line 97: * Validates that all the domains have enough space for the request Line 98: * @return {@link ValidationResult#VALID} if all the domains have enough free space, or a {@link ValidationResult} with the first low-on-space domain encountered. Line 99: */ Line 100: public ValidationResult allDomainsHaveSpaceForNewDisks(final List<DiskImage> disksList) { Line 101: setDomainsDisksMap(disksList); As noted above, this should be a local variable. Line 102: return validOrFirstFailure(new ValidatorPredicate() { Line 103: @Override Line 104: public ValidationResult evaluate(StorageDomainValidator validator) { Line 105: List disksForDomain = getDomainsDisksMap().get(validator.getDomainId()); Line 101: setDomainsDisksMap(disksList); Line 102: return validOrFirstFailure(new ValidatorPredicate() { Line 103: @Override Line 104: public ValidationResult evaluate(StorageDomainValidator validator) { Line 105: List disksForDomain = getDomainsDisksMap().get(validator.getDomainId()); Instead of adding things to the validation API, juts change the predicate to receive the MapEntry instead of its value. Line 106: return validator.hasSpaceForNewDisks(disksForDomain); Line 107: } Line 108: }); Line 109: } Line 106: return validator.hasSpaceForNewDisks(disksForDomain); Line 107: } Line 108: }); Line 109: } Line 110: TWS Line 111: /** @return The lazy-loaded validator for the given map entry */ Line 112: protected StorageDomainValidator getStorageDomainValidator(Map.Entry<Guid, StorageDomainValidator> entry) { Line 113: if (entry.getValue() == null) { Line 114: entry.setValue(new StorageDomainValidator(getStorageDomainDAO().getForStoragePool(entry.getKey(), storagePoolId))); Line 108: }); Line 109: } Line 110: Line 111: /** @return The lazy-loaded validator for the given map entry */ Line 112: protected StorageDomainValidator getStorageDomainValidator(Map.Entry<Guid, StorageDomainValidator> entry) { why? Line 113: if (entry.getValue() == null) { Line 114: entry.setValue(new StorageDomainValidator(getStorageDomainDAO().getForStoragePool(entry.getKey(), storagePoolId))); Line 115: } Line 116: http://gerrit.ovirt.org/#/c/29258/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 31: } Line 32: Line 33: public Guid getDomainId() { Line 34: return storageDomain.getId(); Line 35: } See comment on MultipleStorageDomainsValidator Line 36: Line 37: public ValidationResult isDomainExistAndActive() { Line 38: if (storageDomain == null) { Line 39: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST); http://gerrit.ovirt.org/#/c/29258/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java: Line 269: @Test Line 270: public void testAllDomainsHaveSpaceForNewDisksFailure() { Line 271: setUpGeneralValidations(); Line 272: setUpDiskValidations(); Line 273: List disksList = getNonEmptyDiskList(); Generics? Line 274: doReturn(disksList).when(cmd).getDisksList(); Line 275: doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)).when(multipleStorageDomainsValidator) Line 276: .allDomainsHaveSpaceForNewDisks(disksList); Line 277: assertFalse(cmd.canDoAction()); Line 277: assertFalse(cmd.canDoAction()); Line 278: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForNewDisks(disksList); Line 279: assertTrue(cmd.getReturnValue() Line 280: .getCanDoActionMessages() Line 281: .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN.name())); Please replace this assert (and the previous one) with a CanDoActionTestUtils call. Line 282: } Line 283: Line 284: @Test Line 285: public void testAllDomainsHaveSpaceForNewDisksSuccess() { Line 287: setUpDiskValidations(); Line 288: List disksList = getNonEmptyDiskList(); Line 289: doReturn(disksList).when(cmd).getDisksList(); Line 290: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsHaveSpaceForNewDisks(disksList); Line 291: assertTrue(cmd.canDoAction()); Please replace this assert with a CanDoActionTestUtils call. Line 292: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForNewDisks(disksList); Line 293: } Line 294: Line 295: private void setUpDiskValidations() { http://gerrit.ovirt.org/#/c/29258/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java: Line 46: Line 47: private Guid spId; Line 48: Line 49: private static Guid sdId1; Line 50: private static Guid sdId2; why?! Line 51: Line 52: private StorageDomain domain1; Line 53: private StorageDomain domain2; Line 54: -- To view, visit http://gerrit.ovirt.org/29258 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0a5b5df695d30a34fbdef31da2320b322e27466b Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
