Yair Zaslavsky has posted comments on this change.
Change subject: engine : Introduction of CommandManager
......................................................................
Patch Set 7: (17 inline comments)
....................................................
File backend/manager/modules/bll/pom.xml
Line 120:
Line 121: <dependency>
Line 122: <groupId>net.sf.ehcache</groupId>
Line 123: <artifactId>ehcache</artifactId>
Line 124: <version>2.6.5</version>
Please move the hard coded 2.6.5 version to the versions section at root pom
(see for example how slf4j version is handled).
Line 125: <type>pom</type>
Line 126: </dependency>
Line 127:
Line 128: </dependencies>
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CacheProviderFactory.java
Line 6: import net.sf.ehcache.config.PersistenceConfiguration;
Line 7: import net.sf.ehcache.store.MemoryStoreEvictionPolicy;
Line 8:
Line 9: public class CacheProviderFactory {
Line 10: private static final int MAX_ELEMENTS = 50;
Maybe have this configurable at db?
Line 11: private static CacheManager manager = CacheManager.create();
Line 12:
Line 13: public static <K, V> CacheWrapper<K, V> getCacheWrapper(String
name) {
Line 14: return new EhcacheWrapperImpl<K, V>(getCache(name));
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.LRU)
What made you change this from LFU to LRU? Do you want to have a discussion on
this?
I thought of LRU as from what I understand what effects more on the cache is
the time between command execution to when command ends (i.e - the cache is not
accessed on high frequency per command).
What do you think?
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/CacheWrapper.java
Line 7: V get(K key);
Line 8:
Line 9: void remove(final K key);
Line 10:
Line 11: public boolean containsKey(K key);
no need for "public" , if for other methods you did not write "public"
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandEntityDAO.java
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4: import org.ovirt.engine.core.compat.NGuid;
Line 5:
Line 6:
Line 7: public interface CommandEntityDAO {
I would introduce CommandEntity + CommandEntityDAO in separate patches in order
to make the patchset more readable.
Thanks.
In addition, not sure I understand - isn't this supposed to be DAO for db?
Line 8:
Line 9: public CommandEntity get(NGuid stepId);
Line 10:
Line 11: public void remove(NGuid stepId);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandEntity.java
Line 10: private NGuid parentCommandId;
Line 11: private VdcActionType actionType;
Line 12: private VdcActionParametersBase parameters;
Line 13:
Line 14: public NGuid getCommandId() {
Guid or NGuid? Can CommandId be null?
(I know, NGuid is kinda redundant, but still).
Line 15: return commandId;
Line 16: }
Line 17:
Line 18: public void setCommandId(NGuid commandId) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandManager.java
Line 1: package org.ovirt.engine.core.bll.tasks;
I would consider even having a sub package for command management, but not
mandatory.
Line 2:
Line 3: import java.util.ArrayList;
Line 4: import java.util.HashMap;
Line 5: import java.util.Map;
Line 43: import org.ovirt.engine.core.utils.log.LogFactory;
Line 44: import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil;
Line 45: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 46:
Line 47: public class CommandManager implements CallBack, Poller, TaskHelper {
1. Barak and I talked about the name of CommandManager, and he disklikes it.
Maybe CommandRepository?
2. Why "implements Poller"? Don't we want to have several pollers?
Line 48:
Line 49: private static final String EXECUTION_CONTEXT_MAP_NAME =
"executionContext";
Line 50: private static final CommandManager instance = new
CommandManager();
Line 51: private static final Log log =
LogFactory.getLog(CommandManager.class);
Line 52:
Line 53: public static CommandManager getInstance() {
Line 54: return instance;
Line 55: }
Line 56: CacheWrapper<NGuid, ExecutionContext> executionContextMap;
As explained earlier - I think Eli might need a map of commandId to
executionContext as well for the external tasks injection feature.
Please coordinate with him this , and how he can reuse your work.
Line 57: CommandEntityDAO commandEntityDAO = new CommandEntityDAOImpl();
Line 58:
Line 59: private CommandManager() {
Line 60: executionContextMap = CacheProviderFactory.<NGuid,
ExecutionContext>getCacheWrapper(EXECUTION_CONTEXT_MAP_NAME);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/DecoratedCommand.java
Line 7: import org.ovirt.engine.core.common.interfaces.TaskHelper;
Line 8: import org.ovirt.engine.core.utils.log.Log;
Line 9: import org.ovirt.engine.core.utils.log.LogFactory;
Line 10:
Line 11: public class DecoratedCommand<T extends VdcActionParametersBase>
implements Command {
Can you please elaborate in comments on the class why this is needed?
I would also consider to have a baseCommandDecorator, and perhaps have another
one with the concrete implmenetation you suggested extend it.
Line 12:
Line 13: private Command command;
Line 14: private AsyncTasks dbAsyncTask;
Line 15: private static final Log log =
LogFactory.getLog(DecoratedCommand.class);
Line 30: dbAsyncTask);
Line 31: return vdcReturnValue;
Line 32: }
Line 33:
Line 34: private static void handleEndActionResult(
Why is this private static?
Line 35: TaskHelper taskHelper,
Line 36: EntityMultiAsyncTasks entityInfo,
Line 37: VdcReturnValueBase vdcReturnValue,
Line 38: AsyncTasks dbAsyncTask) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/EntityAsyncTask.java
Line 17: import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil;
Line 18:
Line 19: /**
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
where is the definition of OnAfterEntityTaskEnded?
In addition, this seems to me like an interim step in the refactor/coding. I
don't think we should rely on "entities" anymore, but on parent commands.
We should move code from here to command manager, if possible.
Line 22: * other tasks regarding the relevant entity have already ended.
Line 23: */
Line 24: public class EntityAsyncTask extends SPMAsyncTask {
Line 25:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/TaskManagerUtil.java
Line 75: }
Line 76:
Line 77: private static AsyncTaskManager getAsyncTaskManager() {
Line 78: return AsyncTaskManager.getInstance(
Line 79: CommandManager.getInstance(),
Why pass the same parameter 3 times?
Can't we change the getInstance method of AsncTaskManager so this will not be
needed (i.e - change it to accept CommandManager and that's it?)
Line 80: CommandManager.getInstance(),
Line 81: CommandManager.getInstance());
Line 82: }
Line 83:
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/BackwardCompatibilityTaskCreationTest.java
Line 162: assertEquals("wrong task ID", info.getTaskID(),
spmAsyncTask.getTaskID());
Line 163: assertEquals("wrong task result",
AsyncTaskResultEnum.success, spmAsyncTask.getLastTaskStatus().getResult());
Line 164: assertEquals("wrong task status", AsyncTaskStatusEnum.init,
spmAsyncTask.getLastTaskStatus().getStatus());
Line 165: assertEquals("wrong task state", AsyncTaskState.Initializing,
spmAsyncTask.getState());
Line 166: assertTrue("wrong task type",
spmAsyncTask.isEntityAsyncTask());
As mentioned before,
I would be very happy if we get rid of the notion of "entity async task".
Line 167: }
Line 168:
Line 169: /**
Line 170: * Tests that a purely engine command, with no async tasks throws
the
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/interfaces/CallBack.java
Line 5: import org.ovirt.engine.core.common.businessentities.AsyncTasks;
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, AsyncTasks dbAsyncTask);
1. Isnt this too general name for the interface ("Callback"), or maybe too
concrete manem for the method? ("endAction" - do we always want to "endAction"
here? think about the requirement for subsequent tasks).
2. Not sure what "stepId" is and why do you need it. If this is an interim
stage at the refactor, please state that at inline comment at the code, thanks.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/interfaces/Poller.java
Line 4: import org.ovirt.engine.core.common.businessentities.AsyncTaskStatus;
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6:
Line 7: public interface Poller {
Line 8: Map<Guid, AsyncTaskStatus> getAllTasksStatuses(Guid storagePoolID);
I assume this will change in the future, right?
The scheduling framework will use a Callable, right?
Line 9: AsyncTaskStatus getTaskStatus(Guid storagePoolID, Guid taskID);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/interfaces/Task.java
Line 5: import org.ovirt.engine.core.common.asynctasks.AsyncTaskState;
Line 6: import org.ovirt.engine.core.common.businessentities.AsyncTaskStatus;
Line 7: import org.ovirt.engine.core.compat.Guid;
Line 8:
Line 9: public interface Task {
I kinda dislike this interface -
a. The name suggests this is a generic task, but clearly the implementation is
SPM tightly-coupled. If this is an interim stage - please add an inline comment
at the code.
b. I thought that Task will be a base class for holding relevant fields (i.e -
taskId) and not defining the operations.
Line 10:
Line 11: void concreteStartPollingTask();
Line 12:
Line 13: void startPollingTask();
--
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: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
