Arik Hadas has posted comments on this change. Change subject: engine: remove the use of @LockIdNameAttribute ......................................................................
Patch Set 8: Code-Review-1 (2 comments) The verification of this patch is difficult because of the following scenario: let's say you have a command that didn't have the LockIdNameAttribute but inherits from a command that has this annotation. before this patch this command won't try to lock (because there is no @Inherited on LockIdNameAttribute so it is not inherited) and now it will (unless it overrides the getLockingPropertiesSettings method and return Scope.None). we have such cases (at least one): AddVmAndAttachToPoolCommand didn't have the LockIdNameAnnotation but extends AddVmCommand that had it and since you don't override the getLockingPropertiesSettings method there, this command will now try to lock the resources which are defined in AddVmCommand. You should basically go over all the commands in the system to see there are no more similar cases 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 6: scope I don't think the terminology is right - it's not about scoping but about duration. the previous flag just said if the lock should be automatically be removed after the execute stage or automatically removed after the end-action phase, in both cases you could release it manually if you like. so I think it should be renamed to something like 'duration' with values which reflect the possible alternative well and the default should be to remove at the end of the execution as it used to be. 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; why the default value is true? it used to be false and it made sense to set it to false by default because only in some specific rare cases we want to wait, in general we want to fail the command immediately if we can't lock the required resources. it also makes more sense that attribute won't be set by default and set if only if necessary. 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
