Liron Aravot has posted comments on this change. Change subject: core: add disk profile to disk's commands ......................................................................
Patch Set 3: Code-Review-1 (9 comments) please move the code that repeats itself to a helper to to a method that'll be used by all the commands. marking -1 for visibility. http://gerrit.ovirt.org/#/c/29038/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java: Line 111: if (!validate(diskValidator.isReadOnlyPropertyCompatibleWithInterface())) { Line 112: return false; Line 113: } Line 114: Line 115: if (!setAndValidateDiskProfiles()) { move this to be within the if in line 119 Line 116: return false; Line 117: } Line 118: Line 119: if (DiskStorageType.IMAGE == getParameters().getDiskInfo().getDiskStorageType()) { Line 589: } Line 590: Line 591: protected boolean setAndValidateDiskProfiles() { Line 592: setDiskProfileParameters(); Line 593: return validate(new DiskProfileValidator(getDiskProfileId()).isStorageDomainValid(getStorageDomainId())); for LUN disk this will fail with NPE. Line 594: } Line 595: Line 596: protected void setDiskProfileParameters() { Line 597: if (getDiskImageInfo() != null Line 600: if (diskProfileId == null) { Line 601: List<DiskProfile> allForStorageDomain = Line 602: getDiskProfileDao().getAllForStorageDomain(getStorageDomainId()); Line 603: if (allForStorageDomain.size() == 1) { Line 604: getDiskImageInfo().setDiskProfileId(allForStorageDomain.get(0).getId()); why would we want to select random profile? Line 605: } Line 606: } Line 607: } Line 608: } http://gerrit.ovirt.org/#/c/29038/3/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 580: for (final DiskImage diskImage : diskInfoDestinationMap.values()) { Line 581: if (!diskProfileValidationMap.containsKey(diskImage.getDiskProfileId())) { Line 582: diskProfileValidationMap.put(diskImage.getDiskProfileId(), Line 583: new DiskProfileValidator(diskImage.getDiskProfileId())); Line 584: } if it exists in the map, it means that it was already validated so no need to validate it again. Line 585: if (!validate(diskProfileValidationMap.get(diskImage.getDiskProfileId()) Line 586: .isStorageDomainValid(diskImage.getStorageIds() Line 587: .get(0)))) { Line 588: return false; Line 581: if (!diskProfileValidationMap.containsKey(diskImage.getDiskProfileId())) { Line 582: diskProfileValidationMap.put(diskImage.getDiskProfileId(), Line 583: new DiskProfileValidator(diskImage.getDiskProfileId())); Line 584: } Line 585: if (!validate(diskProfileValidationMap.get(diskImage.getDiskProfileId()) you may get an NPE here if the diskimage has no profile id. Line 586: .isStorageDomainValid(diskImage.getStorageIds() Line 587: .get(0)))) { Line 588: return false; Line 589: } Line 1235: storageDomainDiskProfileMap.put(storageDomainId, Line 1236: getDiskProfileDao().getAllForStorageDomain(storageDomainId)); Line 1237: } Line 1238: List<DiskProfile> storageDomainDiskProfiles = storageDomainDiskProfileMap.get(storageDomainId); Line 1239: if (storageDomainDiskProfiles.size() == 1) { what if there are more than one profiles for the domain? Line 1240: diskImage.setDiskProfileId(storageDomainDiskProfiles.get(0).getId()); Line 1241: } Line 1242: } Line 1243: } Line 1236: getDiskProfileDao().getAllForStorageDomain(storageDomainId)); Line 1237: } Line 1238: List<DiskProfile> storageDomainDiskProfiles = storageDomainDiskProfileMap.get(storageDomainId); Line 1239: if (storageDomainDiskProfiles.size() == 1) { Line 1240: diskImage.setDiskProfileId(storageDomainDiskProfiles.get(0).getId()); why do we set a random one? Line 1241: } Line 1242: } Line 1243: } Line 1244: } http://gerrit.ovirt.org/#/c/29038/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java: Line 398: return doClusterRelatedChecks(); Line 399: } Line 400: } Line 401: Line 402: protected boolean setAndValidateDiskProfiles() { please export this code to a utility or to a method that will be used by multiple commands, no need to repeat it and maintain it multiple times. Line 403: setDiskProfileParameters(); Line 404: final Map<Guid, DiskProfileValidator> diskProfileValidationMap = new HashMap<>(); Line 405: for (final DiskImage diskImage : diskInfoDestinationMap.values()) { Line 406: if (!diskProfileValidationMap.containsKey(diskImage.getDiskProfileId())) { Line 802: } Line 803: return diskImage.getQuotaId(); Line 804: } Line 805: Line 806: protected void setDiskProfileParameters() { same Line 807: Map<Guid, List<DiskProfile>> storageDomainDiskProfileMap = new HashMap<>(); Line 808: for (DiskImage diskImage : diskInfoDestinationMap.values()) { Line 809: if (diskImage.getDiskProfileId() == null) { Line 810: Guid storageDomainId = diskImage.getStorageIds().get(0); -- To view, visit http://gerrit.ovirt.org/29038 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7b6d977243cffc0bde772665ebaca47340075c6 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Doron Fediuck <[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
