Vered Volansky has posted comments on this change. Change subject: core: Add space validation when creating snapshot ......................................................................
Patch Set 4: (12 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 state Done 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. Done 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 o Done 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 Done 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. Done 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 t Done 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 Done 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? In order to be able to mock 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/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? Done 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 CanDoActionTestUti Done 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. Done 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?! For the use in static generateDisksList. Why not? 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
