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

On 09/27/2012 01:11 AM, Ayal Baron wrote: > > > ----- Original Message ----- >> >> >> ----- 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. IMHO, I think the fact ALL commands have the ability to create VDSM async tasks (even nowadays, regardless of the serial execution suggestion) - is bad. IMHO, A command should use AsyncTaskManager to create/manage tasks

>>> >>> 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. Who knows if storage will be the only case. See previous comment on where we should consider having task MGMT code.

>>> >>>> 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. > > I'm saying nacking something without suggesting a better > alternative is not good practice. > > Besides, the design is classic callback pattern > (http://en.wikipedia.org/wiki/Callback_%28computer_programming%29) > which is quite common and accepted. > Regardless though, take a look at the new patches. > >> >>>> >>>> >>>>>> 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 >>>>>> >>>>> >>>> >>> >> > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel >