Alon Bar-Lev has posted comments on this change. Change subject: core: introduce context pattern ......................................................................
Patch Set 37: (2 comments) http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java: Line 597: getVmStaticDAO().incrementDbGeneration(getVmTemplateId()); Line 598: for (VdcActionParametersBase p : getParameters().getImagesParameters()) { Line 599: Backend.getInstance().endAction(VdcActionType.CreateImageTemplate, Line 600: p, Line 601: getContext().duplicate().resetCompensationContext().resetExecutionContext().resetLock()); > isn't there dupContext() method in CommandBase ? even if there is, we need the new context which is duplicate of our context customized sent to the child. Line 602: } Line 603: if (reloadVmTemplateFromDB() != null) { Line 604: endDefaultOperations(); Line 605: } http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 170: protected CommandBase(T parameters, CommandContext cmdContext) { Line 171: if (cmdContext == null) { Line 172: cmdContext = Line 173: new CommandContext(new EngineContext().setSessionId(parameters.getSessionId())) Line 174: .setExecutionContext(new ExecutionContext()); > this is bit hard to follow. I'd extract the EngineContext into local variab what is the difference if you use temp variable or have it inline? in both cases it is very clear that it is EngineContext either by variable name or the new EngineContext(), just without extra statement. I usually use better indentation... but it seems that the auto formatter we use is invalid.... should be: cmdContext = new CommandContext( new EngineContext().setSessionId(parameters.getSessionId()) ).setExecutionContext(new ExecutionContext()); but again, this is a matter of style.... not that important. I just hate temp variables. Line 175: } Line 176: this.context = cmdContext; Line 177: _parameters = parameters; Line 178: DbUser user = -- 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: 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
