----- Original Message ----- > > > On 09/27/2012 01:11 AM, Ayal Baron wrote: > > > > > > ----- 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. > 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
Fine by me, but that's a whole different ball game and has nothing to do with the current feature. > >>> > >>> 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. I agree it should be split out of the commands altogether, as most of the code in commands should (in fact I see no justification for the command pattern in most places in the code at all). But that is a much bigger change and requires a ton of refactoring. > > > >>> > >>>> 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 > > > _______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
