Allon Mureinik has posted comments on this change. Change subject: core: Storage domain attaching validator addition. ......................................................................
Patch Set 2: Code-Review-1 (10 comments) http://gerrit.ovirt.org/#/c/36287/2//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-12-21 12:13:32 +0200 Line 6: Line 7: core: Storage domain attaching validator addition. Line 8: Line 9: Storage domain attaching validations was extracted to a new validator. s/was/were/ Line 10: Storage domain validations was extracted as well to its proper Line 11: validator. Line 12: Line 13: Change-Id: I079a51b5ddc36805a945ffc59965101418187441 Line 6: Line 7: core: Storage domain attaching validator addition. Line 8: Line 9: Storage domain attaching validations was extracted to a new validator. Line 10: Storage domain validations was extracted as well to its proper s/was/were/ Line 11: validator. Line 12: Line 13: Change-Id: I079a51b5ddc36805a945ffc59965101418187441 http://gerrit.ovirt.org/#/c/36287/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/AttachDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/AttachDomainValidator.java: Line 17: Line 18: /** Line 19: * CanDoAction validation methods for attaching a storage domain to a DC (pool). Line 20: */ Line 21: public class AttachDomainValidator { "Domain" may be a bit confusing - how about "AttachStorageDomainValidator" ? Line 22: private final StorageDomain storageDomain; Line 23: private final StoragePool storagePool; Line 24: private final boolean isStorageDomainOfTypeIsoOrExport; Line 25: private final StorageDomainValidator domainValidator; Line 20: */ Line 21: public class AttachDomainValidator { Line 22: private final StorageDomain storageDomain; Line 23: private final StoragePool storagePool; Line 24: private final boolean isStorageDomainOfTypeIsoOrExport; Why do we need this as a member variable? Line 25: private final StorageDomainValidator domainValidator; Line 26: private final StoragePoolValidator poolValidator; Line 27: Line 28: public AttachDomainValidator(StorageDomain domain, StoragePool pool) { Line 22: private final StorageDomain storageDomain; Line 23: private final StoragePool storagePool; Line 24: private final boolean isStorageDomainOfTypeIsoOrExport; Line 25: private final StorageDomainValidator domainValidator; Line 26: private final StoragePoolValidator poolValidator; This looks like a breakage in abstraction - if someone wants to verify the domain or the pool, they should use those validators. THIS validatior should be concerned with the relationship between the domain and the pool only. Line 27: Line 28: public AttachDomainValidator(StorageDomain domain, StoragePool pool) { Line 29: storageDomain = domain; Line 30: storagePool = pool; Line 47: } Line 48: Line 49: public ValidationResult isStorageDomainCompatibleWithDC() { Line 50: if (storageDomain.getStorageType() == StorageType.GLUSTERFS) { Line 51: return getPoolValidator().isGlusterSupportedInDC(); This logic should just be moved here - there's no reason to have it StoragePoolValidator. Line 52: } Line 53: Line 54: if (storageDomain.getStorageType() == StorageType.POSIXFS) { Line 55: return getPoolValidator().isPosixSupportedInDC(); Line 51: return getPoolValidator().isGlusterSupportedInDC(); Line 52: } Line 53: Line 54: if (storageDomain.getStorageType() == StorageType.POSIXFS) { Line 55: return getPoolValidator().isPosixSupportedInDC(); Same here. Line 56: } Line 57: Line 58: return ValidationResult.VALID; Line 59: } Line 79: public boolean eval(StorageDomain a) { Line 80: return a.getStorageDomainType() == type; Line 81: } Line 82: } Line 83: ).size(); I understand this is just migrated code, but... ichs. There has to be a better way of doing this - probably in a different patch, though. Line 84: Line 85: // If the count is zero we are okay, we can add a new one: Line 86: if (count == 0) { Line 87: return ValidationResult.VALID; http://gerrit.ovirt.org/#/c/36287/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/AttachDomainValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/AttachDomainValidatorTest.java: Line 38: import static org.mockito.Mockito.when; Line 39: import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; Line 40: Line 41: @RunWith(MockitoJUnitRunner.class) Line 42: public class AttachDomainValidatorTest { Should be renamed together with the class it tests. Line 43: StorageDomain storageDomain; Line 44: StoragePool storagePool; Line 45: AttachDomainValidator validator; Line 46: StorageDomainValidator domainValidator; Line 50: StoragePoolDAO storagePoolDAO; Line 51: @Mock Line 52: StorageDomainDAO storageDomainDAO; Line 53: @Mock Line 54: StoragePoolIsoMapDAO storagePoolIsoMapDAO; All of the above members should be private Line 55: Line 56: @Before Line 57: public void setUp() throws Exception { Line 58: // Create the storage domain. -- To view, visit http://gerrit.ovirt.org/36287 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I079a51b5ddc36805a945ffc59965101418187441 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
