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

----- 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.

>> >> 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. 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