Yair Zaslavsky has posted comments on this change.
Change subject: engine : Introduction of CommandManager
......................................................................
Patch Set 4: (11 inline comments)
First few comments, will add more later...
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CacheProviderFactory.java
Line 26: private static Cache initGetCache(final String name) {
Line 27: //Create a Cache specifying its configuration.
Line 28: Cache cache = new Cache(
Line 29: new CacheConfiguration(name, MAX_ELEMENTS)
Line 30:
.memoryStoreEvictionPolicy(MemoryStoreEvictionPolicy.LFU)
I wonder if it should be LFU or LRU.
Line 31: .eternal(true)
Line 32: .diskExpiryThreadIntervalSeconds(0)
Line 33: .persistence(new
PersistenceConfiguration().strategy(PersistenceConfiguration.Strategy.LOCALTEMPSWAP)));
Line 34: return cache;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandManager.java
Line 54: CacheWrapper<NGuid, CommandEntity> commandMap;
Line 55:
Line 56: private CommandManager() {
Line 57: executionContextMap = CacheProviderFactory.<NGuid,
ExecutionContext>getCacheWrapper(EXECUTION_CONTEXT_MAP_NAME);
Line 58: commandMap = CacheProviderFactory.<NGuid,
CommandEntity>getCacheWrapper(COMMAND_MAP_NAME);
Bare in mind that for External task injection feature, Eli is going to
introduce a map from jobId to ExecutionContext.
In addition, if ExecutionContext gets evicted (due to LFU - btw, should this be
LFU or LRU? ) - how are we going to restore the context due to "cache miss"?
can you please elaborate?
Line 59: }
Line 60:
Line 61: @Override
Line 62: public VdcReturnValueBase endAction(
Line 161: task.setAssociatedEntities(entityIds);
Line 162: AsyncTaskUtils.addOrUpdateTaskInDB(task);
Line 163: commandMap.put(
Line 164: task.getParameters().getDbAsyncTask().getStepId(),
Line 165: buildGetCommandEntity(
Hi,
Don't we want to put the entire hierarchy that created the task at the commands
map?
Line 166: command.getCommandId(),
Line 167: command.getParameters().getParentParameters() ==
null ? Guid.Empty :
command.getParameters().getParentParameters().getCommandId(),
Line 168: task));
Line 169: getAsyncTaskManager().lockAndAddTaskToManager(task);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/interfaces/CallBack.java
Line 5: import org.ovirt.engine.core.common.action.VdcReturnValueBase;
Line 6: import org.ovirt.engine.core.compat.NGuid;
Line 7:
Line 8: public interface CallBack {
Line 9: public VdcReturnValueBase endAction(NGuid stepId, VdcActionType
actionType, VdcActionParametersBase actionParameters);
Hi,
Maybe later on the Callback will have the signature of
void taskEnded(Task task) - and it will the logic of endAction should be in
CommandManager that will implement the CallBack - what do u think?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/interfaces/TaskHelper.java
Line 14: */
Line 15: public interface TaskHelper {
Line 16: void endTaskStep(NGuid stepId, JobExecutionStatus exitStatus);
Line 17: void endTaskJob(NGuid stepId, boolean exitStatus);
Line 18: VDSReturnValue RunVdsCommand(VDSCommandType commandType,
VDSParametersBase parameters);
As explained before, RunVDSCommand should be removed -
Instead - relevant interfaces (or Functors, "functions") should be provided to
the places in task mgr that need to run vds broker.
Line 19: boolean taskHasContext(NGuid stepId);
Line 20: public void acquireLockAsyncTask(AsyncTaskParameters
actionParameters);
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/AsyncTaskFactory.java
Line 14: import org.ovirt.engine.core.common.interfaces.TaskHelper;
Line 15: import org.ovirt.engine.core.utils.log.Log;
Line 16: import org.ovirt.engine.core.utils.log.LogFactory;
Line 17:
Line 18: public final class AsyncTaskFactory {
Later on this will move back to BLL.
We might have a base class at taskmgr to give some of the generic functionality.
Line 19: /**
Line 20: * Constructs a task based on creation info (task type and task
parameters
Line 21: * as retrieved from the vdsm). Use in order to construct tasks
when service
Line 22: * is initializing.
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/AsyncTaskManager.java
Line 1: package org.ovirt.engine.core.taskmgr;
Later on this will become the SPM service, and will move back to BLL.
Line 2:
Line 3: import java.util.ArrayList;
Line 4: import java.util.HashMap;
Line 5: import java.util.HashSet;
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/EntityAsyncTask.java
Line 20: * EntityAsyncTask: Base class for all tasks regarding a specific
entity (VM,
Line 21: * VmTemplate). The 'OnAfterEntityTaskEnded' method will be executed
only if all
Line 22: * other tasks regarding the relevant entity have already ended.
Line 23: */
Line 24: public class EntityAsyncTask extends SPMAsyncTask {
Looks like most of the code here will be moved to CommandManager.
Please check and see how you can break the code of EntityAsyncTask to parts
which will be reused by EntityAsyncTask (for commands that will not move at the
begining to the new framework) and by CommandManager (for example - the code
around endAction).
Line 25:
Line 26: private static final Object _lockObject = new Object();
Line 27:
Line 28: private static final Map<Object, EntityMultiAsyncTasks>
_multiTasksByEntities =
Line 162: Ex2);
Line 163: }
Line 164:
Line 165: finally {
Line 166: handleEndActionResult(taskHelper, entityInfo,
vdcReturnValue, dbAsyncTask);
Using Roy's idea of interceptor -
Perhaps -we should consider intercepting Backend.endAction and use it to end
the job in order to have cleaner code.
Line 167: _endActionsInProgress.decrementAndGet();
Line 168: }
Line 169: }
Line 170:
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/SPMAsyncTask.java
Line 379: AsyncTaskStatus returnValue = null;
Line 380:
Line 381: try {
Line 382: Object tempVar = taskHelper
Line 383: .RunVdsCommand(VDSCommandType.SPMGetTaskStatus,
As explained yesterday,
At the end the code of SPMAsyncTask should be removed ->
It's code should be moved to the SPM service.
What I suggest is that you provide here "poller function" - even if you
currently use some sort of a Functor, or define an interface of Poller, and
then later on we will move to our scheduling framework I think it will mark us
that this is the direction we're going to.
Line 384: new
SPMTaskGuidBaseVDSCommandParameters(getStoragePoolID(),
getTaskID())).getReturnValue();
Line 385: returnValue = (AsyncTaskStatus) ((tempVar instanceof
AsyncTaskStatus) ? tempVar : null);
Line 386: }
Line 387:
Line 449: getTaskID(),
Line 450:
(getParameters().getDbAsyncTask().getaction_type()),
Line 451: getParameters().getClass().getName());
Line 452:
Line 453: taskHelper.RunVdsCommand(VDSCommandType.SPMStopTask,
Same as above for stop task.
This way this way we can get read of RunVdsCommand in taskHelper.
Line 454: new
SPMTaskGuidBaseVDSCommandParameters(getStoragePoolID(), getTaskID()));
Line 455: } catch (RuntimeException e) {
Line 456: log.error(
Line 457: String.format("SPMAsyncTask::StopTask:
Stopping task '%1$s' threw an exception.", getTaskID()),
--
To view, visit http://gerrit.ovirt.org/13152
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibdd7585bfcfa6adeb761a8532218ba1aaa5e3c5d
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
