Re: [Engine-devel] Serial Execution of Async Tasks

----- Original Message ----- > From: "Ayal Baron" <abaron(a)redhat.com&gt; > To: "Michael Kublin" <mkublin(a)redhat.com&gt; > Cc: "Liron Aravot" <laravot(a)redhat.com&gt;, "engine-devel" > <engine-devel(a)ovirt.org&gt;, "Eduardo Warszawski" > <ewarszaw(a)redhat.com&gt;, "Allon Mureinik" <amureini(a)redhat.com&gt; > Sent: Sunday, September 23, 2012 5:27:54 PM > Subject: Re: [Engine-devel] Serial Execution of Async Tasks > > > > ----- Original Message ----- > > > > > > ----- Original Message ----- > > > From: "Ayal Baron" <abaron(a)redhat.com&gt; > > > To: "Allon Mureinik" <amureini(a)redhat.com&gt;, "Michael Kublin" > > > <mkublin(a)redhat.com&gt; > > > Cc: "Liron Aravot" <laravot(a)redhat.com&gt;, "engine-devel" > > > <engine-devel(a)ovirt.org&gt;, "Eduardo Warszawski" > > > <ewarszaw(a)redhat.com&gt; > > > Sent: Sunday, September 23, 2012 1:10:07 PM > > > Subject: Re: [Engine-devel] Serial Execution of Async Tasks > > > > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > ----- Original Message ----- > > > > > From: "Michael Kublin" <mkublin(a)redhat.com&gt; > > > > > To: "Allon Mureinik" <amureini(a)redhat.com&gt; > > > > > Cc: "Eduardo Warszawski" <ewarszaw(a)redhat.com&gt;, "Liron > > > > > Aravot" > > > > > <laravot(a)redhat.com&gt;, "Maor Lipchuk" > > > > > <mlipchuk(a)redhat.com&gt;, "engine-devel" > > > > > <engine-devel(a)ovirt.org&gt; > > > > > Sent: Sunday, September 23, 2012 12:41:05 PM > > > > > Subject: Re: [Engine-devel] Serial Execution of Async Tasks > > > > > > > > > > > > > > > >>>>>>>>>> Hi guys, > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> As you may know the engine currently has the > > > > > >>>>>>>>>> ability > > > > > >>>>>>>>>> to > > > > > >>>>>>>>>> fire > > > > > >>>>>>>>>> an > > > > > >>>>>>>>>> SPM > > > > > >>>>>>>>>> task, and be asynchronously be "woken-up" when > > > > > >>>>>>>>>> it > > > > > >>>>>>>>>> ends. > > > > > >>>>>>>>>> This is great, but we found the for the Live > > > > > >>>>>>>>>> Storage > > > > > >>>>>>>>>> Migration > > > > > >>>>>>>>>> feature we need something a bit complex - the > > > > > >>>>>>>>>> ability > > > > > >>>>>>>>>> to > > > > > >>>>>>>>>> have a > > > > > >>>>>>>>>> series of async tasks in a single control flow. > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> Here's my initial design for this, your comments > > > > > >>>>>>>>>> and > > > > > >>>>>>>>>> criticism > > > > > >>>>>>>>>> would > > > > > >>>>>>>>>> be welcome: > > > > > >>>>>>>>>> http://wiki.ovirt.org/wiki/Features/Serial_Execution_of_Asynchronous_Task... > > > > > >>>>>>>> -successful execution - > > > > > >>>>>>>> * "CommandBase iterates over its > > > > > >>>>>>>> SPMAsyncTaskHandlers" > > > > > >>>>>>>> - > > > > > >>>>>>>> when? > > > > > >>>>>>> This is the new suggested format of > > > > > >>>>>>> executeCommand(). > > > > > >>>>>>> I'll > > > > > >>>>>>> clarify > > > > > >>>>>>> this too. > > > > > >>>>>>> > > > > > >>>>>>>> * If the second task is an HSM command (vs. SPM > > > > > >>>>>>>> command), > > > > > >>>>>>>> I > > > > > >>>>>>>> think you > > > > > >>>>>>>> should explain in the design how to handle such > > > > > >>>>>>>> flows > > > > > >>>>>>>> as > > > > > >>>>>>>> well. > > > > > >>>>>>> HSM commands do not create AsyncTasks, as they do > > > > > >>>>>>> today > > > > > >>>>>>> - > > > > > >>>>>>> I > > > > > >>>>>>> will > > > > > >>>>>>> clarify this. > > > > > >>>>>>> > > > > > >>>>>>>> * Why do we need before task? can you give a > > > > > >>>>>>>> concrete > > > > > >>>>>>>> example > > > > > >>>>>>>> of what > > > > > >>>>>>>> would you do in such a method. > > > > > >>>>>>> Basically, /today/, command look like this: > > > > > >>>>>>> executeCommand() { > > > > > >>>>>>> doStuffInTheDB(); > > > > > >>>>>>> runVdsCommand(someCommand); > > > > > >>>>>>> } > > > > > >>>>>>> > > > > > >>>>>>> endSuccessfully() { > > > > > >>>>>>> doMoreStuffInTheDB(); > > > > > >>>>>>> } > > > > > >>>>>>> > > > > > >>>>>>> endWithFailure() { > > > > > >>>>>>> doMoreStuffForFailureInTheDB(); > > > > > >>>>>>> } > > > > > >>>>>>> > > > > > >>>>>>> In the new design, the entire doStuffInTheDB() > > > > > >>>>>>> should > > > > > >>>>>>> be > > > > > >>>>>>> moved > > > > > >>>>>>> to a > > > > > >>>>>>> breforeTask of the (only) SPMAsyncTaskHandler. > > > > > >>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> - I see you added SPMAsyncTaskHandler, any reason > > > > > >>>>>>>> not > > > > > >>>>>>>> to > > > > > >>>>>>>> use > > > > > >>>>>>>> SPMAsyncTasK to manage it own life-cycle? > > > > > >>>>>>> Conserving today's design - The SPMAsyncTaskHandler > > > > > >>>>>>> is > > > > > >>>>>>> the > > > > > >>>>>>> place to > > > > > >>>>>>> add additional, non-SPM, logic around the SPM task > > > > > >>>>>>> execution, > > > > > >>>>>>> like > > > > > >>>>>>> CommandBase allows today. > > > > > >>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> - In the life-cycle managed by the > > > > > >>>>>>>> SPMAsyncTaskHandler > > > > > >>>>>>>> there > > > > > >>>>>>>> is a > > > > > >>>>>>>> step > > > > > >>>>>>>> 'createTask - how to create the async task' can > > > > > >>>>>>>> you > > > > > >>>>>>>> please > > > > > >>>>>>>> elaborate > > > > > >>>>>>>> what are the options. > > > > > >>>>>>> new [any type of async task] > > > > > > > > > > (I cleaned thread a little.) > > > > > The following design and it is implementation > > > > > http://gerrit.ovirt.org/#/c/7956/ > > > > > is bad. > > > > > I don't see any reason for creating a new > > > > > SPMAsyncTaskHandler > > > > > and > > > > > especially in the > > > > > way as it's done in patch. > > > > > The reason are following: > > > > > 1. Performance , increased memory footprint, created CYCLIC > > > > > REFERENCE. > > > > > 2. Readability and robust of code: the code which is > > > > > written > > > > > as > > > > > cyclic references is unreadable > > > > > and difficult for debug. > > > > > 3. Why I need a generic implementation and changes all over > > > > > whole > > > > > project because of > > > > > series of async commands, for me it is a private case? > > > > > > What is the private case here exactly? > > > Every task can have multiple jobs. We've identified several > > > such > > > places (e.g. live storage migration, move disk, move vm) and I > > > have > > > no doubt more will popup. > > > As Allon notes below, task handling is done at CommandBase, if > > > you > > > think task management should be for storage only, you're > > > welcome > > > to > > > push it down to StorageHandlingCommandBase (or get rid of > > > inheritance here altogether). > > Interesting , regards cyclic reference no response, but who > > cares, > > it is difficult to answer , that's why better not to response? > > There is no problem with cyclic references in general, GCs know how > to deal with these just fine and in this case it's limited to the > command and its handlers. > I did not reply, however, as I do not feel strongly about this. > > > Regards private case: > > 1. We have command that not creating any task > > 2. We have command that will create a one task. > > 3. And we have 3 commands meanwhile which will create more than > > one > > task. > > I think that 3 is private case and not common? (In the future, I > > once happens > twice is a coincidence > three times is a method > > But if you insist on more then it's easy enough. We've discussed > many times in the past that we need to change many of the storage > verbs to run asynchronously (e.g. createStorageDomain) once this > happens then existing flows would have to run multiple async tasks > serially. > > > removed too many > > lines of code that were preparation for future that never come) > > This is not in preparation for the future, it is for a feature > we're > working on right now (live storage migration) and for fixing move > disk on which we have several bugs pending. > > > The handling done at CommandBase it means that it is influence > > all > > system. > > That is how the task management was done. Again, if you feel it > should only affect storage flows, feel free to push it down into > StorageCommandHandlingBase and then only storage verbs will have > task management. > > > Now regards architecture why I need some handler which will be > > inside > > a command > > and will call for command methods? Please explain. > > As opposed to what? So, u think it is a good design where we are using a "Circular references" design pattern. If yes, please elaborate.

> > > > > > > > This will occur all over the storage commands (which are the > > > > only > > > > usages of tasks nowadys). > > > > Moreover, async task handling is done at the Commandbase > > > > level > > > > (see > > > > the end* methods) - instead of hacking it in X different > > > > places > > > > whenever we need it, I'd prefer doing it once, properly. > > > > > > > > > > > > > > _______________________________________________ > > > > Engine-devel mailing list > > > > Engine-devel(a)ovirt.org > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > > > > >