Alon Bar-Lev has posted comments on this change. Change subject: aaa: Adding engineSessionId on CommandContext ......................................................................
Patch Set 9: (20 comments) http://gerrit.ovirt.org/#/c/28829/9/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 190 Line 191 Line 192 Line 193 Line 194 what about calling CommandBase with null as parameters? Line 1983 Line 1984 Line 1985 Line 1986 Line 1987 I would like to remove this function, can we now? Line 2157: VdcActionParametersBase parameters, Line 2158: CommandContext internalCommandContext) { Line 2159: return Backend.getInstance().runInternalAction(actionType, Line 2160: parameters, Line 2161: internalCommandContext); the parameter can be kept context... as it is the context in which the action will run. Line 2162: } Line 2163: Line 2164: protected ArrayList<VdcReturnValueBase> runInternalMultipleActions(VdcActionType actionType, Line 2165: ArrayList<VdcActionParametersBase> parameters) { http://gerrit.ovirt.org/#/c/28829/9/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 183: } Line 184: ThreadLocalParamsContainer.setCorrelationId(command.getCorrelationId()); Line 185: command.insertAsyncTaskPlaceHolders(); Line 186: if (commandContext != null) { Line 187: command.getContext().setEngineSessionId(commandContext.getEngineSessionId()); why do we ned this? it should be inherited. Line 188: } Line 189: command.executeAction(); Line 190: } Line 191: Line 189: command.executeAction(); Line 190: } Line 191: Line 192: public void setCommandContext(CommandContext commandContext) { Line 193: this.commandContext = commandContext; can we drop the set and pass this in constructor or something? Line 194: } Line 195: Line 196: public void setIsRunOnlyIfAllCanDoPass(boolean isRunOnlyIfAllCanDoPass) { Line 197: this.isRunOnlyIfAllCanDoPass = isRunOnlyIfAllCanDoPass; http://gerrit.ovirt.org/#/c/28829/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/NetworkClusterAttachmentActionRunner.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/NetworkClusterAttachmentActionRunner.java: Line 53: Line 54: // multiple networks can be either attached or detached from a single cluster Line 55: if (!params.isEmpty()) { Line 56: Backend.getInstance().runInternalAction(massAction, Line 57: new ClusterNetworksParameters(params.get(0).getVdsGroupId(), params), commandContext); hmmm... I guess people will expect getter from parent, no? Line 58: } Line 59: } http://gerrit.ovirt.org/#/c/28829/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestartVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestartVdsCommand.java: Line 126 Line 127 Line 128 Line 129 Line 130 why not duplicate? http://gerrit.ovirt.org/#/c/28829/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java: Line 247: ExecutionContext executionContext = new ExecutionContext(); Line 248: executionContext.setMonitored(true); Line 249: Backend.getInstance().runInternalMultipleActions(VdcActionType.MigrateVmToServer, Line 250: new ArrayList<>(createMigrateVmToServerParametersList(vmsToMigrate, vds)), Line 251: new CommandContext(executionContext)); why not duplicate? Line 252: } Line 253: } catch (RuntimeException e) { Line 254: log.errorFormat("Failed to initialize Vds on up. Error: {0}", e); Line 255: } http://gerrit.ovirt.org/#/c/28829/9/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 23 Line 24 Line 25 Line 26 Line 27 we do not need this... as it equals to new CommandContext().setEngineSessionId(xxx) Line 33 Line 34 Line 35 Line 36 Line 37 we do not need this as well Line 43 Line 44 Line 45 Line 46 Line 47 and I think this as well Line 55 Line 56 Line 57 Line 58 Line 59 also this Line 68 Line 69 Line 70 Line 71 Line 72 and this Line 79 Line 80 Line 81 Line 82 Line 83 and this Line 84 Line 85 Line 86 Line 87 Line 88 and this :))) Line 98: } Line 99: Line 100: public CommandContext(VdcActionParametersBase params, CommandContext context) { Line 101: if (context == null) { Line 102: engineSessionId = params.getSessionId(); I would also have added if params != null Line 103: } else { Line 104: setFromContext(context); Line 105: } Line 106: Line 101: if (context == null) { Line 102: engineSessionId = params.getSessionId(); Line 103: } else { Line 104: setFromContext(context); Line 105: } but... I would have done this: if (params != null) { engineSessionId = params.getSessionId(); } setFromContext(context); and in setFromContext() add if (context != null). why? as you always first set whatever you can from parameters, then override all from context, the override can put defaults and such... Line 106: Line 107: } Line 108: Line 109: private void setFromContext(CommandContext context) { Line 115: public CommandContext duplicate() { Line 116: CommandContext ctx = new CommandContext(); Line 117: ctx.setFromContext(this); Line 118: return ctx; Line 119: } please support clonebale interface, and use clone(). Line 120: Line 121: public String getEngineSessionId() { Line 122: return engineSessionId; Line 123: } http://gerrit.ovirt.org/#/c/28829/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interfaces/BackendInternal.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interfaces/BackendInternal.java: Line 79: Line 80: /** Line 81: * Execute the pool monitoring job immediately Line 82: */ Line 83: void triggerPoolMonitoringJob(); not move this? Line 84: http://gerrit.ovirt.org/#/c/28829/9/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 530: } Line 531: Line 532: public static CommandContext createDefaultContexForTasks(ExecutionContext parentContext, EngineLock lock) { Line 533: ExecutionContext executionContext = createDefaultContextForTasksImpl(parentContext); Line 534: CommandContext commandContext = new CommandContext(executionContext).setLock(lock); I would like to duplicate existing context. Line 535: return commandContext; Line 536: } Line 537: Line 538: /** -- 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: 9 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: Oved Ourfali <[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
