Yair Zaslavsky has posted comments on this change. Change subject: core: introduce context pattern ......................................................................
Patch Set 37: (18 comments) Arik, many thanks for the review! 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? i guess not. 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/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 f should be in command base. command context should not know what end action is. what do u think? 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? Done 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 Done 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 Done 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? Done 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 comma that's a good point, however the caller of dup should reset the lock if needed. 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 (s are you sure you can do that? it should override the clone method of Object. 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 hmm, good idea. 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? see my comment before on this. 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? Done 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/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)? Done 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)? Done 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)? Done 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)? Done 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)? Done 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)? Done 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 Done 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 <[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
