Gilad Chaplik has posted comments on this change. Change subject: core: disk profile commands and queries ......................................................................
Patch Set 10: (17 comments) http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/AddDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/AddDiskProfileCommand.java: Line 8: import org.ovirt.engine.core.common.VdcObjectType; Line 9: import org.ovirt.engine.core.common.action.DiskProfileParameters; Line 10: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 11: import org.ovirt.engine.core.compat.Guid; Line 12: > please make non transactive. when we'll solve the bug of the "default non t again, transaction is irrelevant for this context - it's common throughout the application. if that's bothering you so much, please solve the bug. Line 13: public class AddDiskProfileCommand extends DiskProfileCommandBase { Line 14: Line 15: public AddDiskProfileCommand(DiskProfileParameters parameters) { Line 16: super(parameters); Line 27: Line 28: @Override Line 29: protected void executeCommand() { Line 30: getParameters().getProfile().setId(Guid.newGuid()); Line 31: getDiskProfileDao().save(getParameters().getProfile()); > how do we handle synchronization here? for example - what if we add a profi DB handles synchronization. for your example I'll get FK constraint error which is fine. Line 32: getReturnValue().setActionReturnValue(getParameters().getProfile().getId()); Line 33: setSucceeded(true); Line 34: } Line 35: Line 34: } Line 35: Line 36: @Override Line 37: public List<PermissionSubject> getPermissionCheckSubjects() { Line 38: return Collections.singletonList(new PermissionSubject(getParameters().getProfile().getStorageDomainId(), > if the profile isn't set you'll have NPE here. Done Line 39: VdcObjectType.Storage, getActionType().getActionGroup())); Line 40: } Line 41: Line 42: @Override http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileCommandBase.java: Line 18: if (profile == null) { Line 19: if (getParameters().getProfile() != null) { Line 20: profile = getParameters().getProfile(); Line 21: } Line 22: else if (getParameters().getProfileId() != null) { > please move the else a line up Done Line 23: profile = getProfileDao().get(getParameters().getProfileId()); Line 24: } else { Line 25: log.error("couldn't find disk profile"); Line 26: } Line 21: } Line 22: else if (getParameters().getProfileId() != null) { Line 23: profile = getProfileDao().get(getParameters().getProfileId()); Line 24: } else { Line 25: log.error("couldn't find disk profile"); > - no need for that, same as we do not log if it's not found in the db. the CDA will fail on null. will remove log. Line 26: } Line 27: } Line 28: return profile; Line 29: } Line 34: profileId = getParameters().getProfileId(); Line 35: } else if (getParameters().getProfile() != null) { Line 36: profileId = getParameters().getProfile().getId(); Line 37: } else { Line 38: log.error("couldn't find disk profile id"); > same.. will remove log. Line 39: } Line 40: } Line 41: return profileId; Line 42: } http://gerrit.ovirt.org/#/c/28732/10/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 15: Line 16: public class DiskProfileValidator { Line 17: Line 18: private final DiskProfile diskProfile; Line 19: private DiskProfile oldDiskProfile; > please rename to diskProfileFromDb why? Line 20: private StorageDomain storageDomain; Line 21: private List<DiskProfile> diskProfiles; Line 22: Line 23: // private List<DiskImage> diskImages; Line 19: private DiskProfile oldDiskProfile; Line 20: private StorageDomain storageDomain; Line 21: private List<DiskProfile> diskProfiles; Line 22: Line 23: // private List<DiskImage> diskImages; > ? will change at a later patch. but will remove it. Line 24: Line 25: public DiskProfileValidator(DiskProfile diskProfile) { Line 26: this.diskProfile = diskProfile; Line 27: } Line 72: } Line 73: Line 74: // public ValidationResult diskProfileNotUsedByDisks() { Line 75: // return diskProfileNotUsed(getDisksUsingProfile(), VdcBllMessages.VAR__ENTITIES__VMS); Line 76: // } > ? same Line 77: Line 78: protected ValidationResult diskProfileNotUsed(List<? extends Nameable> entities, VdcBllMessages entitiesReplacement) { Line 79: if (entities.isEmpty()) { Line 80: return ValidationResult.VALID; Line 107: } Line 108: Line 109: return oldDiskProfile; Line 110: } Line 111: > ? same Line 112: // protected List<VM> getDisksUsingProfile() { Line 113: // if (diskImages == null) { Line 114: // diskImages = getDbFacade().getDiskImageDao().getAllForDiskProfile(diskProfile.getId()); Line 115: // } http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/RemoveDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/RemoveDiskProfileCommand.java: Line 19: protected boolean canDoAction() { Line 20: DiskProfileValidator validator = new DiskProfileValidator(getProfile()); Line 21: return validate(validator.diskProfileIsSet()) Line 22: && validate(validator.diskProfileExists()); Line 23: // && validate(validator.diskProfileNotUsedBydisks()) > why don't we check if it's used or not? DB handles synchronization. will fix commented code. Line 24: } Line 25: Line 26: @Override Line 27: protected void executeCommand() { Line 30: } Line 31: Line 32: @Override Line 33: public List<PermissionSubject> getPermissionCheckSubjects() { Line 34: return Collections.singletonList(new PermissionSubject(getParameters().getProfile().getStorageDomainId(), > NPE if the profile isn't set (checked on CDA) Done Line 35: VdcObjectType.Storage, getActionType().getActionGroup())); Line 36: } Line 37: Line 38: @Override http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/UpdateDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/UpdateDiskProfileCommand.java: Line 21: return validate(validator.diskProfileIsSet()) Line 22: && validate(validator.diskProfileExists()) Line 23: && validate(validator.diskProfileNameNotUsed()) Line 24: && validate(validator.storageDomainNotChanged()) Line 25: && validate(validator.qosExistsOrNull()); > what if there are running vms whose disks use that profile? we should atlea message should be propagated by command that does the actual change. this is just dataentry, no changes will take place. Line 26: } Line 27: Line 28: @Override Line 29: protected void executeCommand() { http://gerrit.ovirt.org/#/c/28732/10/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 37: return ValidationResult.VALID; Line 38: } Line 39: Line 40: public ValidationResult isDomainExistAndActive() { Line 41: if (storageDomain == null) { > please replace those lines with call to the method you added.. Done Line 42: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST); Line 43: } Line 44: if (storageDomain.getStatus() != StorageDomainStatus.Active) { Line 45: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2, http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 995: USER_UPDATED_QOS(10114), Line 996: USER_FAILED_TO_UPDATE_QOS(10115, AuditLogSeverity.ERROR), Line 997: Line 998: // Disk Profile Line 999: USER_ADDED_DISK_PROFILE(10120), > AuditLogMessages.properties is missing in this patch since I'm missing tons of localization for all patches, I will add all in a separate patch. the flow for adding a single string for 4 files is unbearable. this will save a lot of time, don't worry about it. Line 1000: USER_FAILED_TO_ADD_DISK_PROFILE(10121, AuditLogSeverity.ERROR), Line 1001: USER_REMOVED_DISK_PROFILE(10122), Line 1002: USER_FAILED_TO_REMOVE_DISK_PROFILE(10123, AuditLogSeverity.ERROR), Line 1003: USER_UPDATED_DISK_PROFILE(10124), http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/DiskProfileParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/DiskProfileParameters.java: Line 9: Line 10: public DiskProfileParameters() { Line 11: } Line 12: Line 13: public DiskProfileParameters(DiskProfile diskProfile, Guid diskProfileId) { > can't we use the id within the diskProfile object? since for remove we're using only ID, and for add ID is null, I prefer to have these fields separated. Line 14: super(diskProfile, diskProfileId); Line 15: } http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 1002: // profiles Line 1003: ACTION_TYPE_FAILED_PROFILE_NOT_EXISTS(ErrorType.BAD_PARAMETERS), Line 1004: ACTION_TYPE_FAILED_PROFILE_NAME_IN_USE(ErrorType.BAD_PARAMETERS), Line 1005: ACTION_TYPE_FAILED_CANNOT_CHANGE_PROFILE(ErrorType.BAD_PARAMETERS), Line 1006: ACTION_TYPE_FAILED_PROFILE_IN_USE(ErrorType.BAD_PARAMETERS), > messages files are missing in this patch same. Line 1007: Line 1008: // Affinity Groups Line 1009: AFFINITY_GROUP_NAME_TOO_LONG(ErrorType.BAD_PARAMETERS), Line 1010: AFFINITY_GROUP_NAME_INVALID(ErrorType.BAD_PARAMETERS), -- To view, visit http://gerrit.ovirt.org/28732 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d871f1603671bc14ce88c68f85638b1af67f5e1 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[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
