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