Allon Mureinik has posted comments on this change. Change subject: core: Reorganize sd space validations in AddVm ......................................................................
Patch Set 1: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/26733/1//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-02-04 16:31:19 +0200 Line 4: Commit: Vered Volansky <vvola...@redhat.com> Line 5: CommitDate: 2014-04-13 11:50:28 +0300 Line 6: Line 7: core: Reorganize sd space validations in AddVm s/sd/SD/ Line 8: Line 9: This is the first of two patches meant to fix faulty storage space Line 10: allocations validations in the different add VM commands. Line 11: This patch keeps the current (faulty) validations and reorganizes them http://gerrit.ovirt.org/#/c/26733/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 263: Line 264: protected boolean canAddVm(ArrayList<String> reasons, Collection<StorageDomain> destStorages) { Line 265: VmStatic vmStaticFromParams = getParameters().getVmStaticData(); Line 266: boolean canAddVm = Line 267: canAddVm(reasons, vmStaticFromParams.getName(), getStoragePoolId(), vmStaticFromParams.getPriority()); If you're already using the early return pattern, might as well use it throughout the code: if (!canAddVm(...)) { return false; } Line 268: Line 269: if (canAddVm) { Line 270: List<ValidationError> validationErrors = validateCustomProperties(vmStaticFromParams); Line 271: if (!validationErrors.isEmpty()) { Line 350: protected StorageDomainValidator createStorageDomainValidator(StorageDomain storageDomain) { Line 351: return new StorageDomainValidator(storageDomain); Line 352: } Line 353: Line 354: private boolean validateDomainsThreshold(StorageDomainValidator storageDomainValidator){ formatting - missing space before "{" Line 355: return validate(storageDomainValidator.isDomainWithinThresholds()); Line 356: } Line 357: Line 358: protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, StorageDomain domain) Line 355: return validate(storageDomainValidator.isDomainWithinThresholds()); Line 356: } Line 357: Line 358: protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, StorageDomain domain) Line 359: { take it up a line please Line 360: return validate(storageDomainValidator.isDomainHasSpaceForRequest(getNeededDiskSize(domain.getId()))); Line 361: } Line 362: Line 363: protected boolean checkSingleQxlDisplay() { http://gerrit.ovirt.org/#/c/26733/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java: Line 293 Line 294 Line 295 Line 296 Line 297 This cleanup is unrelated to the patch - please perform it in a more appropriate patch. Line 223: assertTrue("isVirtioScsiEnabled hasn't been defaulted to true on cluster >= 3.3.", cmd.isVirtioScsiEnabled()); Line 224: } Line 225: Line 226: @Test Line 227: public void ValidateSpaceAndThreshold() { methods should start with lower case Line 228: AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); Line 229: assertTrue(command.validateSpaceRequirements()); Line 230: } Line 231: Line 229: assertTrue(command.validateSpaceRequirements()); Line 230: } Line 231: Line 232: @Test Line 233: public void ValidateSpaceNotWithinThreshold() throws Exception { methods should start with lower case Line 234: AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); Line 235: storageDomainValidator = mock(StorageDomainValidator.class); Line 236: doReturn((new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN))). Line 237: when(storageDomainValidator).isDomainWithinThresholds(); -- To view, visit http://gerrit.ovirt.org/26733 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I482709d7edb0333a6aa7b9ab723eea7f4c5a00d3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@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
