Greg Padgett has posted comments on this change. Change subject: engine : Remove ACTIVE_ASYNC status ......................................................................
Patch Set 6: Code-Review-1 (1 comment) Patch looks fine to me, I agree in principle that the extra states aren't really needed, and that the callback logic can decide whether or not to execute anything. However, see the embedded comment--this breaks Live Merge. (And a general request: if changing the command logic, please add me as a reviewer. I wouldn't have found this except for wondering where the extra states went while reviewing a different patch. Thanks.) http://gerrit.ovirt.org/#/c/30555/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandCallback.java: Line 14 Line 15 Line 16 Line 17 Line 18 This block is important. We use the ACTIVE vs ACTIVE_ASYNC state to indicate whether the command has progressed far enough for doPolling() to take any action. If this method is called prematurely due to not having this safeguard, it may succeed before any of the child commands have had a chance to run. In short: we'll need an alternate method to delay doPolling() (maybe something persisted in the command Parameters, or similar?). -- To view, visit http://gerrit.ovirt.org/30555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I382790a2f4042f2924a0916f62047790bb6d5e94 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[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
