Alissa Bonas has posted comments on this change.
Change subject: core: Extract SD validations from ImagesHandler
......................................................................
Patch Set 6: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java
Line 22: */
Line 23: public class MultipleStorageDomainsValidator {
Line 24:
Line 25: /** The ID of the storage pool all the domains belong to */
Line 26: private NGuid spId;
how about naming the member storagePoolId? much more clear to the reader than
spId. I know there's a comment here, but it shouldn't affect the code being
clear by itself.
Line 27:
Line 28: /** A map from the ids of each domain being validated to its
validator */
Line 29: private Map<Guid, StorageDomainValidator> domainValidators;
Line 30:
Line 35: public MultipleStorageDomainsValidator(NGuid spId,
Collection<Guid> sdIds) {
Line 36: this.spId = spId;
Line 37: domainValidators = new HashMap<Guid,
StorageDomainValidator>(sdIds.size());
Line 38: for (Guid id : sdIds) {
Line 39: domainValidators.put(id, null);
Perhaps a matter of taste, but filling a map with null values is not my cup of
tea.
I'd put a member with set of all storage domain ids, and then as the code is
called to get a storage domain validator, add an entry to the map.
Line 40: }
Line 41: }
Line 42:
Line 43: /**
Line 68:
Line 69: /** @return The lazy-loaded validator for the given map entry */
Line 70: private StorageDomainValidator
getStorageDomainValidator(Map.Entry<Guid, StorageDomainValidator> entry) {
Line 71: if (entry.getValue() == null) {
Line 72: entry.setValue(new
StorageDomainValidator(getStorageDomainDAO().getForStoragePool(entry.getKey(),
spId)));
this code doesn't seem very safe. If the call to db fails, the class
construction might fail, and then it's hard to troubleshoot it, no proper
logging, etc.
Isn't it better to first get the domain from db, and if it returned OK, pass it
to constructor?
Line 73: }
Line 74:
Line 75: return entry.getValue();
Line 76: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java
Line 93: @Test
Line 94: public void testAllDomainsWithinThresholdsOneLacking() {
Line 95: domain1.getStorageDynamicData().setAvailableDiskSize(15);
Line 96: domain2.getStorageDynamicData().setAvailableDiskSize(7);
Line 97: ValidationResult result =
validator.allDomainsWithinThresholds();
maybe I'm not familiar with the subject, but from this test it's not clear what
is the threshold.
Line 98: assertFalse("Both domains should not be withing thresholds",
result.isValid());
Line 99: assertEquals("Wrong validation error",
Line 100:
VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN,
Line 101: result.getMessage());
....................................................
Commit Message
Line 10: ImagesHandler.PerformImagesChecks, and made the relevant commands use
it
Line 11: directly.
Line 12:
Line 13: In order to do this, a new type of validator was introduced,
Line 14: MultipleStoroageDomainsValidator, which aggregates validations from
storoage->storage
Line 15: individual StorageDomainValidators in a fast-fail pattern. This is done
Line 16: for performance reasons, so there is no need to load all the storage
Line 17: domains from the database preemptively.
Line 18:
--
To view, visit http://gerrit.ovirt.org/12249
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cfc895dceed4de520476ed5e3fa9c1c7cfd78f4
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
