Amit Aviram has posted comments on this change. Change subject: core: Limitation addition for downgrading DC's compatible version. ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/35790/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java: Line 213: StoragePoolValidator validator = createStoragePoolValidator(); Line 214: return validate(validator.isNotLocalfsWithDefaultCluster()); Line 215: } Line 216: Line 217: private boolean isDowngradeEnabledByDomains(List<StorageDomainStatic> poolDomains) { > how about "isDowngradeAllowedForDomains" ? Done Line 218: List<String> formatProblematicDomains = new ArrayList<>(); Line 219: List<String> typeProblematicDomains = new ArrayList<>(); Line 220: boolean isMixedSupportFailing = false; Line 221: Line 216: Line 217: private boolean isDowngradeEnabledByDomains(List<StorageDomainStatic> poolDomains) { Line 218: List<String> formatProblematicDomains = new ArrayList<>(); Line 219: List<String> typeProblematicDomains = new ArrayList<>(); Line 220: boolean isMixedSupportFailing = false; Notice that this variable notes that the mixed support causes the validation to *fail*. Hence it is supposed to be false until it finds a domain that fails the check- only then it will note that mixed support DOES fails the check, and get a true value.. it may be confusing- I can change its name.. Line 221: Line 222: for (StorageDomainStatic domainStatic : poolDomains) { Line 223: StorageDomain domain = new StorageDomain(); Line 224: domain.setStorageStaticData(domainStatic); Line 225: Line 226: StorageDomainToPoolRelationValidator attachDomainValidator = getAttachDomainValidator(domain); Line 227: Line 228: if (!attachDomainValidator.isStorageDomainTypeFitsPoolIfMixed().isValid()) { Line 229: isMixedSupportFailing = true; > Don't you want to remember this name somewhere? Done Line 230: } Line 231: if (!attachDomainValidator.isStorageDomainTypeSupportedInPool().isValid()) { Line 232: typeProblematicDomains.add(domain.getName()); Line 233: } Line 235: formatProblematicDomains.add(domain.getName()); Line 236: } Line 237: } Line 238: Line 239: int numberOfProblematicDomainsToShow = 10; > Isn't this the default of ReplacementUtils? It is 5, and anyway- ReplacementUtils comes with only one default function. if I want to use my own separator (which I do), I'll have to give this parameter as well.. Line 240: if (isMixedSupportFailing) { Line 241: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_MIXED_STORAGE_TYPES_NOT_ALLOWED); Line 242: } Line 243: if (!formatProblematicDomains.isEmpty()) { http://gerrit.ovirt.org/#/c/35790/8/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java: Line 443: return DbFacade.getInstance().getVdsGroupDao(); Line 444: } Line 445: } Line 446: Line 447: protected class AttachDomainValidatorForTesting extends StorageDomainToPoolRelationValidator { > Why isn't spying enough here? Because getStoragePoolDao() which I spy is a protected method inside AttachDomainValidator and cannot be reached by this class which is located in another package. Line 448: public AttachDomainValidatorForTesting(StorageDomain domain, StoragePool pool) { Line 449: super(domain, pool); Line 450: } Line 451: Line 449: super(domain, pool); Line 450: } Line 451: Line 452: public StoragePoolDAO getStoragePoolDao() { Line 453: return DbFacade.getInstance().getStoragePoolDao(); > ?! Just a hard copy of the original method.. I think that since usage of this class does not necessarily mocks this function, it is rather be copied like that. I'm aware of the possibility to a code change in the original class but it is a rather straight forward method, so this possibility can be unconsidered.. Line 454: } Line 455: } -- To view, visit http://gerrit.ovirt.org/35790 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib97488c705a32fe73b390ef226b1ad19a3784b76 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
