Gilad Chaplik has posted comments on this change. Change subject: core: add disk profile to disk's commands ......................................................................
Patch Set 3: (10 comments) >please move the code that repeats itself to a helper to to a method that'll be >used by all the commands As I commented out inline, prefer not to, since each storage command behaves differently, and I have bad experience with that. 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 Done 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. Done last comment you gave should cover it. 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? it is not random, this is the only one for that SD. I don't want to disturb any current flows, if there's a single DP in SD, it will be set for the disk. 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 that's exactly what I'm doing. 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. changed 'isStorageDomainValid' to handle that. 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? CDA will fail on empty DP. 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? it's not random, it's the only one for that SD. 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 mu I prefer not to, since each command behave differently, believe me I'd be happy to do that. 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 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); http://gerrit.ovirt.org/#/c/29038/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileValidator.java: Line 23: private StorageDomain storageDomain; Line 24: private List<DiskProfile> diskProfiles; Line 25: Line 26: public DiskProfileValidator(DiskProfile diskProfile) { Line 27: this(diskProfile.getId()); > NPE in case that diskProfile is null Done Line 28: this.diskProfile = diskProfile; Line 29: } Line 30: Line 31: public DiskProfileValidator(Guid diskProfileId) { -- 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: Gilad Chaplik <[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
