Greg Padgett has posted comments on this change. Change subject: [WIP] core, db, packaging: introduce EntityTaskStrategy ......................................................................
Patch Set 2: (8 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 ha Agreed, it'd be nice to hide the enum-to-strategy translation from the task. Line 75: break; Line 76: case EntityTaskStrategy: Line 77: this.strategy = new EntityTaskStrategy(this); Line 78: break; 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 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. I like the idea. I think we'd need both the tasks and the task manager to have some algorithms strategized. I'll go for it once I can take a little time from getting the live merge features coded. Regarding non-SPM task polling, we were told no extra polling [of the host] is allowed, but we do [locally] poll the latest vmStats. Seems like this would fit into a framework like you describe. 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()); > +1 Agreed, the "container" moniker makes sense to me. 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? The task is tied to an entity--in the Live Merge case, a VM. Another option (credit to alitke) is TransientTaskStrategy, or we can try ContainerTaskStrategy (Containerized??) to align with the id/type you suggested before. I'm not picky about the name. :) 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. +1 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 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... To preserve it forever on gerrit. :) I'll remove it. 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? The query in asynctasks.py runs on the system being upgraded before the schema changes take place, so this allows the query to work both on the first upgrade (old version) and on subsequent executions (new version). 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; http://gerrit.ovirt.org/#/c/26913/2/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/upgrade/asynctasks.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/upgrade/asynctasks.py: Line 151: ) Line 152: Line 153: def _getRunningTasks(self, dbstatement): Line 154: Line 155: # TODO GP: any need to try and stop/compensate new-style tasks? > I don't think we should handle upgrade issues in these patches Thanks, I'll remove the TODO marker before submission. I haven't yet worked out the upgrade/recovery flow, but will make sure to handle it in a separate patch rather than this one. Line 156: tasks = dbstatement.execute( Line 157: statement=""" Line 158: select * from inst_get_async_tasks() Line 159: """, -- 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
