Allon Mureinik has posted comments on this change. Change subject: core: ImportVmCommand storage allocation checks ......................................................................
Patch Set 9: Code-Review-1 (4 comments) minor implementation details, see inline. Other than that- looks good! http://gerrit.ovirt.org/#/c/32258/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java: Line 359: } Line 360: return StorageDomainValidator.getSpaceRequirementsForStorageDomains(spaceMap); Line 361: } Line 362: Line 363: protected List<DiskImage> createDiskDummiesForSpaceValidations(List<DiskImage> disksList) { Worth adding a javadoc for this method - it may not be intuitive to a reader outside of the context of this patch Line 364: List<DiskImage> dummies = new ArrayList<>(); Line 365: for (DiskImage image : disksList) { Line 366: Guid targetSdId = imageToDestinationDomainMap.get(image.getId()); Line 367: DiskImage dummy = ImagesHandler.createDiskImageWithExcessData(image, targetSdId); Line 360: return StorageDomainValidator.getSpaceRequirementsForStorageDomains(spaceMap); Line 361: } Line 362: Line 363: protected List<DiskImage> createDiskDummiesForSpaceValidations(List<DiskImage> disksList) { Line 364: List<DiskImage> dummies = new ArrayList<>(); optimization - initialize this with disksList.size() Line 365: for (DiskImage image : disksList) { Line 366: Guid targetSdId = imageToDestinationDomainMap.get(image.getId()); Line 367: DiskImage dummy = ImagesHandler.createDiskImageWithExcessData(image, targetSdId); Line 368: dummies.add(dummy); Line 380: if (getParameters().getCopyCollapse()) { Line 381: return validate(sdValidator.allDomainsHaveSpaceForClonedDisks(disksList)); Line 382: } Line 383: Line 384: return validate(sdValidator.allDomainsHaveSpaceForDisksWithSnapshots(disksList)); suggestion: I'd return the ValidationResult instead of the boolean - it's easier to test that way. Line 385: } Line 386: Line 387: protected MultipleStorageDomainsValidator createMultipleStorageDomainsValidator( List<DiskImage> disksList) { Line 388: return new MultipleStorageDomainsValidator(getStoragePoolId(), http://gerrit.ovirt.org/#/c/32258/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 332: return totalSizeForDisks; Line 333: } Line 334: Line 335: private static interface SizeAssessment { Line 336: public double getSizeForDisk(DiskImage diskImage, double sizeForDisk); Wouldn't "capacity" be a better term than sizeForDisk ? Moreover - why is this a parameter? Isn't it always just extracted from diskImage.getSize() ? Line 337: } -- To view, visit http://gerrit.ovirt.org/32258 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifbb1d985f9afa476452d1d2b78be1fd18c128c8f Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Arik Hadas <[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
