Gilad Chaplik has posted comments on this change. Change subject: core: disk profile commands and queries ......................................................................
Patch Set 10: (4 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 27: Line 28: @Override Line 29: protected void executeCommand() { Line 30: getParameters().getProfile().setId(Guid.newGuid()); Line 31: getDiskProfileDao().save(getParameters().getProfile()); > That's not good enough IMO, in that case the user will get a generic error this is a rare corner case, anyway I've opened a bug for it (bug 1109734). > We never rely on db synchronization all over the application .. imo you should rethink above statement. I suggest you will open 2 parallel webadmin, and start play, lets see how many generic messages you get. >locks are very easy to add -so i don't see why not to add it you're right, easy but imo extremely risky; for corner case nice message it's simply not worth it. Line 32: getReturnValue().setActionReturnValue(getParameters().getProfile().getId()); Line 33: setSucceeded(true); Line 34: } Line 35: 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; > because imo it's clearer than "old" , but that's subjective. Done Line 20: private StorageDomain storageDomain; Line 21: private List<DiskProfile> diskProfiles; Line 22: Line 23: // private List<DiskImage> diskImages; 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()); > i didn't understand the comment. attach/move disk, update disk, or sync SLA policies. If you have N disks that are attached to the specific DP, you want me to set up a message that I've updated #disks? not that practical. Line 26: } Line 27: Line 28: @Override Line 29: protected void executeCommand() { 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), > if it's not added here, it's very hard to review as this patch might "break me too, but it simply time consuming for nothing. The build passes successfully, simply we get keys as error messages. I will try to pass all over the patches before merging and add those, but surely don't want to block 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), -- 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
