----- Original Message ----- > > > ----- Original Message ----- > > From: "Ayal Baron" <[email protected]> > > To: "Michael Kublin" <[email protected]> > > Cc: "Liron Aravot" <[email protected]>, "engine-devel" > > <[email protected]>, "Eduardo Warszawski" > > <[email protected]>, "Allon Mureinik" <[email protected]> > > 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" <[email protected]> > > > > To: "Allon Mureinik" <[email protected]>, "Michael Kublin" > > > > <[email protected]> > > > > Cc: "Liron Aravot" <[email protected]>, "engine-devel" > > > > <[email protected]>, "Eduardo Warszawski" > > > > <[email protected]> > > > > 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" <[email protected]> > > > > > > To: "Allon Mureinik" <[email protected]> > > > > > > Cc: "Eduardo Warszawski" <[email protected]>, "Liron > > > > > > Aravot" > > > > > > <[email protected]>, "Maor Lipchuk" > > > > > > <[email protected]>, "engine-devel" > > > > > > <[email protected]> > > > > > > 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_Tasks_Detailed_Design > > > > > > >>>>>>>> -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 > > > > > [email protected] > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > > > > > > > > > > _______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
