Arik Hadas has posted comments on this change. Change subject: engine: remove the use of @LockIdNameAttribute ......................................................................
Patch Set 8: (2 comments) http://gerrit.ovirt.org/#/c/25944/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LockProperties.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LockProperties.java: Line 2: Line 3: public class LockProperties { Line 4: Line 5: public static enum Scope { Execution, Manual, None } Line 6: private Scope scope = Scope.None; > so you have SCOPE_EXECUTION, SCOPE_ACTION, no? I don't see SCOPE_ACTION anywhere in this patch.. do you mean to add it to the Scope enumeration to distinguish the different durations? if so, it sounds good Line 7: private boolean wait = true; Line 8: Line 9: private LockProperties() {} Line 10: Line 3: public class LockProperties { Line 4: Line 5: public static enum Scope { Execution, Manual, None } Line 6: private Scope scope = Scope.None; Line 7: private boolean wait = true; > the less people need to learn, the less bug you have. so it is better to co That's what I'm concerned of, that the number of bugs that will be created by guys which are not too familiar with the code will be high if the default value won't be the one that should be used most of the time: new guy that adds new command will, most probably, take another command as a reference and will copy the things he needs. if he'll take one of the few commands that need to wait when it try to acquire the lock, he'll create a bug and it won't be easy to understand why. setting the 'wait' to false would mean that it will fit to most of the new commands and if, by mistake, that guy will take command that set 'wait' to true as a reference, it will be easier to understand the mistake because it is explicitly set. I also think that it will be confusing for the reviewers to make such change, assuming they are familiar with the project (and know that in most of the cases you expect that command will fail immediately when the entity it needs to lock is already locked) and are used to the previous code. Line 8: Line 9: private LockProperties() {} Line 10: Line 11: public boolean isWait() { -- To view, visit http://gerrit.ovirt.org/25944 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie57e4f7c00ebcd6a4e9e0e61b7d26f50f2d00858 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
