Moti Asayag has posted comments on this change. Change subject: core: introduce context pattern ......................................................................
Patch Set 34: Code-Review-1 (14 comments) 1. It seems you missed instantiating the commands and queries when those are being invoked internally, with their parent context. 2. Too much noise introduced to MultipleActionRunner and its derivatives 3. The CommandContext and EngineContext implementation is a pojo-builder mixture. While it makes code easier to write, i found it less easier to read (and probably to debug). This is more a matter of style, but i was under the impression that the engine entities comply with the JavaBean spec, which is being violated by the current proposal. Hence i'm in favor of using an inner builder for those classes or use void setters. I'd like to hear other inputs on this one. Besides, nice job! can't wait to see correlation-id being taken care of in next patches. http://gerrit.ovirt.org/#/c/28829/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java: Line 60: commandContext != null ? : would you mind reversing the condition ? (commandContex == null ? ... : ... Line 60: commandContext != null ? Line 61: findCommandConstructor(getCommandClass(action.name(), CommandSuffix), parameters.getClass(), commandContext.getClass()) : Line 62: findCommandConstructor(getCommandClass(action.name(), CommandSuffix), parameters.getClass()); Line 63: Line 64: CommandBase<P> cmd = (CommandBase<P>) constructor.newInstance(new Object[] { parameters }); You've detected the c'tor which expects the commandContext, but you don't use it to instantiate the command ? Line 65: Injector.injectMembers(cmd); Line 66: return cmd; Line 67: } Line 68: catch (InvocationTargetException ex) { Line 113: public static QueriesCommandBase<?> createQueryCommand(VdcQueryType query, VdcQueryParametersBase parameters, EngineContext engineContext) { Line 114: Class<?> type = null; Line 115: try { Line 116: type = getCommandClass(query.name(), QueryPrefix); Line 117: Constructor<?> info = engineContext != null ? findCommandConstructor(type, parameters.getClass(), engineContext.getClass()) : same Line 118: findCommandConstructor(type, parameters.getClass()); Line 119: return (QueriesCommandBase<?>) info.newInstance(parameters); Line 120: } catch (Exception e) { Line 121: log.errorFormat("Command Factory: Failed to create command {0} using reflection\n. {1}", type, e); Line 114: Class<?> type = null; Line 115: try { Line 116: type = getCommandClass(query.name(), QueryPrefix); Line 117: Constructor<?> info = engineContext != null ? findCommandConstructor(type, parameters.getClass(), engineContext.getClass()) : Line 118: findCommandConstructor(type, parameters.getClass()); please remove the extra space Line 119: return (QueriesCommandBase<?>) info.newInstance(parameters); Line 120: } catch (Exception e) { Line 121: log.errorFormat("Command Factory: Failed to create command {0} using reflection\n. {1}", type, e); Line 122: throw new RuntimeException(e); Line 115: try { Line 116: type = getCommandClass(query.name(), QueryPrefix); Line 117: Constructor<?> info = engineContext != null ? findCommandConstructor(type, parameters.getClass(), engineContext.getClass()) : Line 118: findCommandConstructor(type, parameters.getClass()); Line 119: return (QueriesCommandBase<?>) info.newInstance(parameters); as for commands instantiate, you ignore the engineContext when invoking the c'tor. Line 120: } catch (Exception e) { Line 121: log.errorFormat("Command Factory: Failed to create command {0} using reflection\n. {1}", type, e); Line 122: throw new RuntimeException(e); Line 123: } http://gerrit.ovirt.org/#/c/28829/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java: Line 31: protected boolean isRunOnlyIfAllCanDoPass = false; Line 32: Line 33: protected CommandContext commandContext; Line 34: Line 35: public MultipleActionsRunner(VdcActionType actionType, List<VdcActionParametersBase> parameters, CommandContext commandContext, boolean isInternal) { why set the commandContext via the c'tor and not using the setter ? Line 36: this.actionType = actionType; Line 37: this.parameters = parameters; Line 38: this.isInternal = isInternal; Line 39: this.commandContext = commandContext; http://gerrit.ovirt.org/#/c/28829/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java: Line 16: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 17: Line 18: public final class MultipleActionsRunnersFactory { Line 19: public static MultipleActionsRunner createMultipleActionsRunner(VdcActionType actionType, Line 20: ArrayList<VdcActionParametersBase> parameters, all the changes to this file could have been spared if commandContext would set via the setter. Line 21: boolean isInternal, CommandContext commandContext) { Line 22: MultipleActionsRunner runner; Line 23: switch (actionType) { Line 24: case DeactivateStorageDomain: { http://gerrit.ovirt.org/#/c/28829/34/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 48: } Line 49: Line 50: public CommandContext setCompensationContext(CompensationContext compensationContext) { Line 51: this.compensationContext = compensationContext; Line 52: return this; i'm not familiar with such pattern of setter. setter usually set a value and defined as void. If you're calling the setter, you've already got the reference in your hand. why is it needed ? Line 53: } Line 54: Line 55: public CommandContext resetCompensationContext() { Line 56: return setCompensationContext(new DefaultCompensationContext()); Line 61: } Line 62: Line 63: public CommandContext setExecutionContext(ExecutionContext executionContext) { Line 64: this.executionContext = executionContext; Line 65: return this; same here Line 66: } Line 67: Line 68: public CommandContext resetExecutionContext() { Line 69: return setExecutionContext(new ExecutionContext()); Line 74: } Line 75: Line 76: public CommandContext setLock(EngineLock lock) { Line 77: this.lock = lock; Line 78: return this; well...here as well :) Line 79: } Line 80: Line 81: public CommandContext resetLock() { Line 82: return setLock(new EngineLock()); http://gerrit.ovirt.org/#/c/28829/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetAddedGlusterServersQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetAddedGlusterServersQuery.java: Line 95: public VdsGroupDAO getVdsGroupDao() { Line 96: return DbFacade.getInstance().getVdsGroupDao(); Line 97: } Line 98: Line 99: protected BackendInternal getBackend() { this method should be removed, since once moved to runInternalQuery, it became a dead-code (should be removed from the test class as well). Line 100: return Backend.getInstance(); Line 101: } Line 102: http://gerrit.ovirt.org/#/c/28829/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java: Line 509: public static CommandContext createInternalJobContext(EngineLock lock) { Line 510: ExecutionContext executionContext = new ExecutionContext(); Line 511: executionContext.setJobRequired(true); Line 512: executionContext.setMonitored(true); Line 513: return new CommandContext(new EngineContext()).setExecutionContext(executionContext).setLock(lock); why not a proper builder pattern ? (i won't offer overload the c'tor with the various combinations), or assign the new instance to a local variable and act on it. Line 514: } Line 515: Line 516: /** Line 517: * Creates a default execution context for an inner command which creates VDSM tasks so the tasks will be monitored Line 533: * public static CommandContext createDefaultContexForTasks(ExecutionContext parentContext, EngineLock lock) { Line 534: * ExecutionContext executionContext = createDefaultContextForTasksImpl(parentContext); CommandContext Line 535: * commandContext = new CommandContext().setExecutionContext(executionContext).setLock(lock); return commandContext; Line 536: * } Line 537: */ please delete the commented code Line 538: Line 539: /** Line 540: * Creates a default execution context for an inner command which creates VDSM tasks so the tasks will be monitored Line 541: * under the parent {@code StepEnum.EXECUTING} step. If the parent command is an internal command, its parent task Line 547: * The lock which should be released at child command Line 548: * @return A context by which the internal command should be monitored. Line 549: */ Line 550: public static CommandContext createDefaultContextForTasks(CommandContext commandContext, EngineLock lock) { Line 551: return commandContext.duplicate().setLock(lock).resetCompensationContext(); this is a mixture of POJO and build pattern. The CommandContext is just a pojo. It can be changed to a builder pattern, but i'm not in favor of the hybrid solution. Line 552: } Line 553: Line 554: public static void setExecutionContextForTasks(CommandContext commandContext, ExecutionContext executionContext, EngineLock lock) { Line 555: commandContext.setExecutionContext(createDefaultContextForTasksImpl(executionContext)) -- 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: 34 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
