Yair Zaslavsky has posted comments on this change.
Change subject: engine: Support for gluster asynchronous tasks(WIP)
......................................................................
Patch Set 2: (13 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterAsyncCommandBase.java
Line 16:
Line 17: public abstract class GlusterAsyncCommandBase<T extends
GlusterVolumeParameters> extends GlusterVolumeCommandBase<T> {
Line 18:
Line 19: private static final long serialVersionUID = -7411023079841571332L;
Line 20: private Step _asyncTaskStep;
I know there is a mess in the convention, i still prefer without _.
Line 21:
Line 22:
Line 23: public GlusterAsyncCommandBase(T params) {
Line 24: super(params);
Line 28: Map<String, String> values = new HashMap<String, String>();
Line 29: values.put(GlusterConstants.CLUSTER, getVdsGroupName());
Line 30: values.put(GlusterConstants.VOLUME, getGlusterVolumeName());
Line 31: //TODO: Define constants
Line 32: values.put("status", "STARTED" );
Maybe some GlusterTaskStatus (which one of its literals is STARTED) ?
Line 33: values.put("info", " ");
Line 34: return values;
Line 35: }
Line 36:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterTaskManager.java
Line 53: }
Line 54: }
Line 55:
Line 56: private void removeTask(Guid externalTaskId) {
Line 57: if (glusterTasksList.contains(externalTaskId)) {
the contains is really needed? it's not like there will be an exception or
something if you don't use it.
Line 58: glusterTasksList.remove(externalTaskId);
Line 59: }
Line 60: }
Line 61:
Line 59: }
Line 60: }
Line 61:
Line 62: @OnTimerMethodAnnotation("async_task_poll_event")
Line 63: public synchronized void updateJobStepStatusForTasks() {
Can we reduce the synchronization block here?
Line 64: log.info("polling for asyn gluster tasks");
Line 65: List<Guid> toRemoveList = new ArrayList<Guid>();
Line 66: for (Guid taskId : glusterTasksList) {
Line 67: //get status of task from vdsm
Line 69: //get step from db for the corresponding gluster task
Line 70: List<Step> steps =
DbFacade.getInstance().getStepDao().getStepsByExternalId(taskId);
Line 71: //update status in step table
Line 72: for (Step step: steps) {
Line 73: step.setDescription(getTaskMessage(step,task));
Don't you need to update the db after each step update?
Line 74: if (hasTaskCompleted(task)) {
Line 75: //ExecutionHandler.endTaskStep(step.getId(),
getJobCompletedStatus(task));
Line 76:
Line 77: step.markStepEnded(getJobCompletedStatus(task));
Line 81:
JobRepositoryFactory.getJobRepository().updateStep(step);
Line 82: }
Line 83: }
Line 84: }
Line 85: for (Guid removeTaskId : toRemoveList) {
Don't you prefer to do that with iterator over glusterTasksList and use
iterator.remove when nedded?
Line 86: removeTask(removeTaskId);
Line 87: }
Line 88:
Line 89: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterTaskStatusUtil.java
Line 5: import
org.ovirt.engine.core.common.businessentities.gluster.GlusterTaskType;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7: import org.ovirt.engine.core.utils.RandomUtils;
Line 8:
Line 9: public class GlusterTaskStatusUtil {
Maybe this should be GlusterTasksUtil ?
Line 10:
Line 11: public static GlusterAsyncTask getTaskStatus(Guid taskId) {
Line 12: //TODO: Call VDS command to get status. Dummy for now
Line 13: GlusterAsyncTask task = new GlusterAsyncTask();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterTaskType.java
Line 6: import org.ovirt.engine.core.common.action.VdcActionType;
Line 7: import org.ovirt.engine.core.common.job.StepEnum;
Line 8:
Line 9: public enum GlusterTaskType {
Line 10:
REBALANCE(VdcActionType.RebalanceGlusterVolume,StepEnum.REBALANCING_VOLUME),
Why are you having this map?
Can't you just have a method at your base class of GlusterTaskType
getGlusterTaskType which will be overriden in the concrete classes?
I think my suggestion will be more clear (Based on my familiarity with VDSM
async tasks + commands in our system I can guess why you did it, but still - I
think adding such a method at the commands themselves will be more clear).
Line 11: REPLACE_BRICK(VdcActionType.ReplaceGlusterVolumeBrick);
Line 12: // REMOVE_BRICK(VdcActionType.DeleteGlusterVolume); //TODO: change
Line 13:
Line 14: private VdcActionType _actionType;
Line 10:
REBALANCE(VdcActionType.RebalanceGlusterVolume,StepEnum.REBALANCING_VOLUME),
Line 11: REPLACE_BRICK(VdcActionType.ReplaceGlusterVolumeBrick);
Line 12: // REMOVE_BRICK(VdcActionType.DeleteGlusterVolume); //TODO: change
Line 13:
Line 14: private VdcActionType _actionType;
One again, I would prefer not to have _ - but this is minor comment.
Line 15: private StepEnum _step;
Line 16:
Line 17: private static Map<VdcActionType, GlusterTaskType> mappings;
Line 18:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StepDaoDbFacadeImpl.java
Line 107: }
Line 108:
Line 109: @Override
Line 110: public List<Step> getAllStepsWithExternalId() {
Line 111: // TODO Auto-generated method stub
Why does that return null?
Still not implemented?
How about throwing NotImplementedException ?
Line 112: return null;
Line 113: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StepDao.java
Line 6: import org.ovirt.engine.core.common.job.JobExecutionStatus;
Line 7: import org.ovirt.engine.core.common.job.Step;
Line 8: import org.ovirt.engine.core.compat.Guid;
Line 9:
Line 10: public interface StepDao extends GenericDao<Step, Guid> {
When adding new method to DAO, please add corresponding test to DAO test (in
this case, StepDaoTest)
Line 11:
Line 12: /**
Line 13: * Check if the {@link Step} with the given id exists or not.
Line 14: *
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/StartRebalanceGlusterVolumeVDSCommand.java
Line 1: package org.ovirt.engine.core.vdsbroker.gluster;
Line 2:
Line 3: import
org.ovirt.engine.core.common.businessentities.gluster.GlusterAsyncTask;
General question -
Why do you prefer to return GlusterAsyncTask, and not just the task ID?
See for example - CopyImageVDSCommand
Thanks
Line 4: import
org.ovirt.engine.core.common.vdscommands.gluster.GlusterVolumeRebalanceVDSParameters;
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6: import
org.ovirt.engine.core.vdsbroker.vdsbroker.GlusterTaskInfoReturnForXmlRpc;
Line 7: import org.ovirt.engine.core.vdsbroker.vdsbroker.StatusForXmlRpc;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/GlusterTaskInfoReturnForXmlRpc.java
Line 11: private final GlusterAsyncTask glusterTask = new
GlusterAsyncTask();
Line 12:
Line 13: public GlusterTaskInfoReturnForXmlRpc(Map<String, Object>
innerMap) {
Line 14: super(innerMap);
Line 15: //TODO: return a GlusterTask from the map
This TODO is still relevant? More fields to get from the map?
Line 16: if (innerMap.containsKey(TASK_ID))
Line 17:
glusterTask.setTaskId(Guid.createGuidFromString((String)innerMap.get(TASK_ID)));
Line 18: }
Line 19:
--
To view, visit http://gerrit.ovirt.org/11840
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a56785edef091bce74e2b7b0ba9a3314f1397f1
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sahina Bose <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
