Arik Hadas has posted comments on this change. Change subject: core: introduce context pattern ......................................................................
Patch Set 37: (3 comments) http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java: Line 905: protected void endSuccessfully() { Line 906: if (isStatelessSnapshotExistsForVm()) { Line 907: getBackend().endAction(VdcActionType.CreateAllSnapshotsFromVm, Line 908: buildCreateSnapshotParametersForEndAction(), Line 909: getContext().duplicate().resetCompensationContext().resetExecutionContext().resetLock()); > should be in command base. command context should not know what end action I'm ok with it being in CommandBase as well Line 910: Line 911: getParameters().setShouldBeLogged(false); Line 912: getParameters().setRunAsStateless(false); Line 913: http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/context/CommandContext.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/context/CommandContext.java: Line 29: private ExecutionContext executionContext; Line 30: Line 31: public CommandContext(CommandContext ctx) { Line 32: this.compensationContext = ctx.compensationContext; Line 33: this.lock = ctx.lock; > that's a good point, however the caller of dup should reset the lock if nee right. the question is whether it is really that way in all the places (I encountered this late in the review process) and it is something that we should detect on future reviews because it is different than what we used to. I'm not saying it is wrong.. Line 34: this.executionContext = ctx.executionContext; Line 35: this.engineContext = ctx.engineContext; Line 36: } Line 37: Line 40: } Line 41: Line 42: public CommandContext duplicate() { Line 43: return (CommandContext)clone(); Line 44: } > are you sure you can do that? yes, but you can change the return type to a different kind of Object (and it make sense since if someone expect to receive Object, the cast to Object will still work) Line 45: Line 46: public EngineContext getEngineContext() { Line 47: return engineContext; Line 48: } -- To view, visit http://gerrit.ovirt.org/28829 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I310f5f77fff78b3232ee77fe63791425fd521516 Gerrit-PatchSet: 37 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
