Arik Hadas has posted comments on this change. Change subject: core: introduce context pattern ......................................................................
Patch Set 37: (25 comments) yair, instead of having all those constructors, where one doesn't get context and one that does, wouldn't it be easier to create the command instance as before and set the context right after in the runInternal methods and drop the constructor with the context? http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddStepCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddStepCommand.java: Line 17: Line 18: protected Job job; Line 19: protected Step parentStep; Line 20: Line 21: protected AddStepCommand() { I didn't find a place where this constructor is called, do we need it? Line 22: super(); Line 23: } Line 24: Line 25: protected AddStepCommand(T parameters) { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InternalMigrateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InternalMigrateVmCommand.java: Line 12: @NonTransactiveCommandAttribute Line 13: public class InternalMigrateVmCommand<T extends InternalMigrateVmParameters> extends MigrateVmCommand<MigrateVmParameters> { Line 14: Line 15: public InternalMigrateVmCommand(T parameters, CommandContext cmdContext) { Line 16: super(new MigrateVmParameters(parameters)); the context should be passed.. Line 17: } Line 18: Line 19: @Override Line 20: protected void executeCommand() { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceVdsCommand.java: Line 149: ctx.setMonitored(true); Line 150: } catch (RuntimeException e) { Line 151: log.error("Failed to create ExecutionContext for MigrateVmCommand", e); Line 152: } Line 153: return dupContext().setExecutionContext(ctx).resetCompensationContext().resetLock(); aaa Line 154: } Line 155: Line 156: @Override Line 157: protected boolean canDoAction() { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java: Line 249: Line 250: private void endCommandActions() { Line 251: getBackend().endAction(getImagesActionType(), Line 252: getParameters(), Line 253: getContext().duplicate().resetCompensationContext().resetExecutionContext().resetLock()); are you sure that we need to reset the execution-context? child commands don't need to close their steps in the end-action in move/copy disk related flows? Line 254: setSucceeded(true); Line 255: } Line 256: Line 257: @Override http://gerrit.ovirt.org/#/c/28829/37/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 63: if (returnValue == null) { Line 64: CommandBase<?> command = isInternal ? Line 65: CommandsFactory.createCommand(actionType, parameter, commandContext.duplicate() Line 66: .resetCompensationContext() Line 67: .resetExecutionContext()) : why reset the execution-context? let's say migrations are fired from within maintenance, it makes sense that they will appear inside the maintenance job right? Line 68: CommandsFactory.createCommand(actionType, parameter); Line 69: Line 70: command.setInternalExecution(isInternal); Line 71: getCommands().add(command); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/PermissionsCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/PermissionsCommandBase.java: Line 23: super(commandId); Line 24: } Line 25: Line 26: public PermissionsCommandBase(T parameters) { Line 27: super(parameters); this(parameters, null) ? Line 28: } Line 29: Line 30: public PermissionsCommandBase(T parameters, CommandContext commandContext) { Line 31: super(parameters, commandContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemovePermissionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemovePermissionCommand.java: Line 23: super(commandId); Line 24: } Line 25: Line 26: public RemovePermissionCommand(T parameters) { Line 27: super(parameters); this(parameters, null)? Line 28: } Line 29: Line 30: public RemovePermissionCommand(T parameters, CommandContext commandContext) { Line 31: super(parameters, commandContext); 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()); how about having a method in CommandContext that will prepare the context for the end-action phase? this line is the same in every endAction call, right? Line 910: Line 911: getParameters().setShouldBeLogged(false); Line 912: getParameters().setRunAsStateless(false); Line 913: Line 952: runStatelessVmCtx.setJob(job); Line 953: // Since run stateless step involves invocation of command, we should set the run stateless vm step as Line 954: // the "beginning step" of the child command. Line 955: runStatelessVmCtx.setStep(runStatelessStep); Line 956: return dupContext().setExecutionContext(runStatelessVmCtx).setCompensationContext(null).setLock(null); why not using the reset methods? Line 957: } Line 958: Line 959: @Override Line 960: protected void endWithFailure() { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java: Line 209: Line 210: VdcReturnValueBase vdcRetValue = runInternalActionWithTasksContext( Line 211: VdcActionType.RemoveVmHibernationVolumes, Line 212: removeVmHibernationVolumesParameters Line 213: ); should be merged with the previous line Line 214: Line 215: for (Guid taskId : vdcRetValue.getInternalVdsmTaskIdList()) { Line 216: TaskManagerUtil.startPollingTask(taskId); Line 217: } http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 411: Line 412: VdcReturnValueBase ret = runInternalActionWithTasksContext( Line 413: VdcActionType.ExtendImageSize, Line 414: createExtendImageSizeParameters() Line 415: ); please merge with previous line Line 416: Line 417: if (ret.getSucceeded()) { Line 418: getReturnValue().getVdsmTaskIdList().addAll(ret.getInternalVdsmTaskIdList()); Line 419: } else { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java: Line 45: super(commandId); Line 46: } Line 47: Line 48: protected VmTemplateCommand(T parameters) { Line 49: super(parameters); this(parameters, null) and drop the next line? Line 50: setVmTemplateId(parameters.getVmTemplateId()); Line 51: Line 52: } Line 53: 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 major change that the lock is now passed by default to child commands because the child command might release it when it finishes the execute stage even if the parent command locks it for the entire command execution, did you verify such flow? 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: } why not just changing the return value of the clone method in this class (sorry if you already discussed it, just point me to the relevant comment if it was discussed) ? Line 45: Line 46: public EngineContext getEngineContext() { Line 47: return engineContext; Line 48: } Line 78: return this; Line 79: } Line 80: Line 81: public CommandContext resetLock() { Line 82: return setLock(new EngineLock()); by default the lock is null, why not set it to null here as well? (just in order to be consistent) Line 83: } Line 84: Line 85: public EngineLock getLock() { Line 86: return lock; http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/context/EngineContext.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/context/EngineContext.java: Line 26: } Line 27: Line 28: public EngineContext duplicate() { Line 29: return (EngineContext) clone(); Line 30: } same - can we just change the return type of clone() and call super.clone? Line 31: http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeCommand.java: Line 237: // Create execution context for setting option Line 238: ExecutionContext setOptionCtx = new ExecutionContext(); Line 239: setOptionCtx.setMonitored(true); Line 240: setOptionCtx.setStep(setOptionStep); Line 241: return dupContext().setExecutionContext(setOptionCtx); reset lock? Line 242: } Line 243: Line 244: private Map<String, String> getOptionValues(GlusterVolumeEntity volume, GlusterVolumeOptionEntity option) { Line 245: Map<String, String> values = new HashMap<String, String>(); http://gerrit.ovirt.org/#/c/28829/37/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); reset compensation? 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 http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStoragePoolWithStoragesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStoragePoolWithStoragesCommand.java: Line 41: @NonTransactiveCommandAttribute(forceCompensation = true) Line 42: public class AddStoragePoolWithStoragesCommand<T extends StoragePoolWithStoragesParameter> extends Line 43: UpdateStoragePoolCommand<T> { Line 44: public AddStoragePoolWithStoragesCommand(T parameters) { Line 45: super(parameters); this(parameters, null)? Line 46: } Line 47: Line 48: public AddStoragePoolWithStoragesCommand(T parameters, CommandContext commandContext) { Line 49: super(parameters, commandContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectStorageToVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectStorageToVdsCommand.java: Line 26: super(parameters, cmdContext); Line 27: } Line 28: Line 29: public ConnectStorageToVdsCommand(T parameters) { Line 30: super(parameters); this(parameters, null)? Line 31: } Line 32: Line 33: @Override Line 34: protected void executeCommand() { http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetAllFromExportDomainQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetAllFromExportDomainQuery.java: Line 21: public abstract class GetAllFromExportDomainQuery <T, P extends GetAllFromExportDomainQueryParameters> Line 22: extends QueriesCommandBase<P> { Line 23: Line 24: public GetAllFromExportDomainQuery(P parameters) { Line 25: super(parameters); this(parameters, null)? Line 26: } Line 27: Line 28: public GetAllFromExportDomainQuery(P parameters, EngineContext engineContext) { Line 29: super(parameters, engineContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsByVmTemplateIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsByVmTemplateIdQuery.java: Line 20: Line 21: private VmTemplate vmTemplate = null; Line 22: Line 23: public GetStorageDomainsByVmTemplateIdQuery(P parameters) { Line 24: super(parameters); this(parameters, null)? Line 25: } Line 26: Line 27: public GetStorageDomainsByVmTemplateIdQuery(P parameters, EngineContext engineContext) { Line 28: super(parameters, engineContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetTemplatesFromExportDomainQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetTemplatesFromExportDomainQuery.java: Line 18: public class GetTemplatesFromExportDomainQuery<P extends GetAllFromExportDomainQueryParameters> Line 19: extends GetAllFromExportDomainQuery<Map<VmTemplate, List<DiskImage>>, P> { Line 20: Line 21: public GetTemplatesFromExportDomainQuery(P parameters) { Line 22: super(parameters); this(parameters, null)? Line 23: } Line 24: Line 25: public GetTemplatesFromExportDomainQuery(P parameters, EngineContext engineContext) { Line 26: super(parameters, engineContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetVmsFromExportDomainQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetVmsFromExportDomainQuery.java: Line 15: public class GetVmsFromExportDomainQuery<P extends GetAllFromExportDomainQueryParameters> Line 16: extends GetAllFromExportDomainQuery<List<VM>, P> { Line 17: Line 18: public GetVmsFromExportDomainQuery(P parameters) { Line 19: super(parameters); this(parameters, null)? Line 20: } Line 21: Line 22: public GetVmsFromExportDomainQuery(P parameters, EngineContext engineContext) { Line 23: super(parameters, engineContext); http://gerrit.ovirt.org/#/c/28829/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java: Line 59: public abstract class StorageHandlingCommandBase<T extends StoragePoolParametersBase> extends CommandBase<T> { Line 60: private List<DiskImage> diskImagesForStorageDomain; Line 61: Line 62: protected StorageHandlingCommandBase(T parameters, CommandContext commandContext) { Line 63: super(parameters); forgot to pass the context Line 64: init(parameters); Line 65: Line 66: } Line 67: -- 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 <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
