Yair Zaslavsky has posted comments on this change. Change subject: [WIP] core, db, packaging: introduce EntityTaskStrategy ......................................................................
Patch Set 2: (10 comments) http://gerrit.ovirt.org/#/c/26913/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskBase.java: Line 70: this.strategyType = strategyType; Line 71: Line 72: switch (strategyType) { Line 73: case SPMTaskStrategy: Line 74: this.strategy = new SPMTaskStrategy(this); worth considering having a strategy factory class for that, so you won't have to change AsyncTaskBase code if future strategies are added, but it's up to you. Line 75: break; Line 76: case EntityTaskStrategy: Line 77: this.strategy = new EntityTaskStrategy(this); Line 78: break; Line 78: break; Line 79: } Line 80: } Line 81: Line 82: public boolean isSpmTask() { see previous question on isSpmTask. Line 83: return strategyType.isSPMStrategy(); Line 84: } Line 85: Line 86: private AsyncTaskState privateState = AsyncTaskState.forValue(0); http://gerrit.ovirt.org/#/c/26913/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java: Line 458: private void updateTaskStatuses( Line 459: Map<Guid, Map<Guid, AsyncTaskStatus>> poolsAllTasksMap) { Line 460: for (AsyncTask task : _tasks.values()) { Line 461: if (task.getShouldPoll()) { Line 462: if (task.isSpmTask()) { hmm, what if you move the code for spm task to the strategy, and introduce a new method for it in the strategy? this way you won't need isSpmTask. what do you think? Line 463: Map<Guid, AsyncTaskStatus> asyncTasksForPoolMap = poolsAllTasksMap Line 464: .get(task.getObjectId()); Line 465: Line 466: // If the storage pool id exists Line 519: * Get a Set of all the storage pool id's of tasks that should pool. Line 520: * Line 521: * @see AsyncTaskBase#getShouldPoll() Line 522: * @return - Set of active tasks. Line 523: */ ok, i understand why you have the isSpmTask. Have you considered having a strategy for AsyncTaskManager, and not (or not just) tasks? i see that getPoolIdsTasks is used by "pollAndUpdateAsyncTasks" - pollAndUpateAsyncTasks sounds to me like a candidate method for such "async task manager" strategy, as I guess with the non SPM tasks you need to support you will also have some polling? Line 524: private Set<Guid> getPoolIdsTasks() { Line 525: Set<Guid> poolsOfActiveTasks = new HashSet<Guid>(); Line 526: Line 527: for (AsyncTask task : _tasks.values()) { Line 525: Set<Guid> poolsOfActiveTasks = new HashSet<Guid>(); Line 526: Line 527: for (AsyncTask task : _tasks.values()) { Line 528: if (task.isSpmTask() && task.getShouldPoll()) { Line 529: poolsOfActiveTasks.add(task.getObjectId()); not sure if i would call it "objectId" - it's too vague. Please think of another term - maybe "task container" (and then a storage pool is also a "task container"). Line 530: } Line 531: } Line 532: return poolsOfActiveTasks; Line 533: } http://gerrit.ovirt.org/#/c/26913/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/EntityTaskStrategy.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/EntityTaskStrategy.java: Line 14: Line 15: /** Line 16: * Strategy for async tasks whose status is ascertained via callbacks to the issuing command. Line 17: */ Line 18: public class EntityTaskStrategy implements AsyncTaskStrategy { Why the name EntityTaskStrategy? Line 19: private final AsyncTask task; Line 20: private CommandCallback command; Line 21: private static final Log log = LogFactory.getLog(EntityTaskStrategy.class); Line 22: http://gerrit.ovirt.org/#/c/26913/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/AsyncTask.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/AsyncTask.java: Line 45: AsyncTaskState getState(); Line 46: Line 47: VdcObjectType getObjectType(); Line 48: Line 49: Guid getObjectId(); ok, i now understand the reasoning for getObjectId. I think this might be a bit confusing. Please think of another term - maybe as I suggested - taskContainer? and in this case, you will have getTaskContainerType and getTaskContainerId what do you think? Line 50: Line 51: Guid getVdsmTaskId(); Line 52: Line 53: /** http://gerrit.ovirt.org/#/c/26913/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/asynctasks/AsyncTaskCreationInfo.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/asynctasks/AsyncTaskCreationInfo.java: Line 61: public void setStepId(Guid stepId) { Line 62: this.stepId = stepId; Line 63: } Line 64: Line 65: public VdcObjectType getObjectType() { see previous comments on ObjectType, ObjectId thanks! Line 66: // Remain backwards compatible with serialized instances that had only storagePoolID Line 67: if (objectType == null && privateStoragePoolID != null) { Line 68: setObjectType(VdcObjectType.StoragePool); Line 69: } Line 90: * it's unsupported by GWT until version 2.6. Instead, we change the accessors to Line 91: * fill in for the new fields at runtime. Line 92: */ Line 93: /* Line 94: private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { so why do you have it in comment? i don't see it in the original code... Line 95: // Translate the old format (with a storagePoolID) to the new format Line 96: in.defaultReadObject(); Line 97: if (getObjectType() == null && getObjectId() == null && getStoragePoolID() != null) { Line 98: setObjectType(VdcObjectType.StoragePool); http://gerrit.ovirt.org/#/c/26913/2/packaging/dbscripts/inst_sp.sql File packaging/dbscripts/inst_sp.sql: Line 79: LANGUAGE plpgsql; Line 80: Line 81: Line 82: -- Query basic info on existing async tasks Line 83: CREATE OR REPLACE FUNCTION inst_get_async_tasks() not sure i understood why this is needed? Line 84: RETURNS TABLE(action_type INTEGER, task_id UUID, started_at TIMESTAMP WITH TIME ZONE, name VARCHAR) Line 85: AS $procedure$ Line 86: DECLARE Line 87: cond_sql TEXT; -- To view, visit http://gerrit.ovirt.org/26913 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59392247ff2caf997030f4cf975273b3bac69c33 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@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
