Hi Dermot,
Thank you very much for your detailed review. Please see
my responses inline.
On 06/ 2/10 08:46 AM, Dermot McCluskey wrote:
Hi Karen,
This is an excellent spec. It gives a very thorough description of
the Engine.
Specific comments:
5.1 - Here, and in several other places, you refer to "initializing"
the checkpoint
objects. My preference would be to say "instantiating" them, which I
think has
a slightly different meaning. In any event, I think it should be made
very clear
that the checkpoint objects do not exist (ie, their constructor has
not been called),
at registration time - this is only done at execute time.
Good point, I meant to say "instantiating" them. Let me look through
the the document
and make all the corrections as needed.
6 - Although you do specify it later, in 13.1, you do not give the
name of the
engine class (InstallEngine) here. I think it should be listed here.
And also, if it's
known at this stage, I'd like to see it's module name given, eg:
"The engine is imported as follows:
from engine.install_engine import InstallEngine"
Sounds good. I have added a new sub-section in section 6 talking about
the module and and class name.
6.3.4 - Is there a reason why *args and **kwargs are not beside each
other in the
param list? It seems like they would normally be listed together.
You are right that they are normally listed together. The reason I did
it this way
is to show that they can specify an optional argument loglevel as
keyword args. My understanding
is that Python does not allow any key word arguments before the arguments.
So, I can not move loglevel=None before *args.
Also, I think you should discuss args and kwargs a bit further here.
For example,
it should be clarified that all the classes that implement
AbstractCheckpoint
may have different arg lists (and keyword param lists) for their
constructors.
However, if the call to register_checkpoint does not exactly match the
expected
number of args, then an exception will be raised when you call
execute_checkpoint() (eg "__init__() takes exactly 3 args (2 given)")
That's a good point. I will give more description here. Also, I think
during registration
time, I can "inspect" the provided argument and the argument list of the
constructor in the checkpoint and make sure they match, and throw
the exception during registration time, instead of during
execute_checkpoint().
6.3.4 - I think the name "checkpoint_obj" is misleading here - it is
not an object, it is
a string which corresponds to the name of a class constructor, right?
You are right. "checkpoint_obj" is not quiet right.
Should also clarify if this param must be a class constructor name, or
can it also
be a regular function name within that module that returns an
AbstractCheckpoint
object?
Even though we no longer require to support a regular function name
within that module,
it could actually be either a class constructor name or a function that
returns an
AbstractCheckpoint object.
What do think of using the name "checkpoint_class"?
Also, in several other places throughout the doc, this value is
referred to as
"checkpoint name", "checkpoint object's class", etc. I think it
should be consistent.
I will look through and make sure everything is consistently refer to as
name of the checkpoint class.
6.3.4 (Exceptions raised list) - does the fact that ImportError is
listed here mean
that you will attempt to import the module during registration,
instead of during
execution (as in the prototype)? If so, will you only import, or will
you also
check for the existence of the constructor method in the attribute
list? Any other
checks performed at this stage?
Since I think it will be best to fail as early as possible if the module
is not found,
I think it is best to load the module and find the constructor, and
possible check
validate the argument list during registration. The actual
instantiation of the object
won't happen until execute_checkpoint() time. I will make this more
clear in section 6.3.
6.4.2 - As this is the first place the ZFS dataset is mentioned, you
could make a forward
reference to say this is discussed in 7.4.
Sounds good, will do.
6.4.3 - I think the name "pause_at" is confusing. It's not completely
clear if it means
before or after. I think "pause_before" is more clear?
Yes, "pause_before" is better. Will change.
6.4.3 (Exceptions raised list) - could there also be a TypeError if
the *args specified
in register_checkpoint don't match the expected list (as mentioned
above)?
As discussed above, I want to actually check for the number of argument
during
registration time. If it doesn't match, I will throw the TypeError there.
6.5 - it's not clear to me exactly how the app can cancel execution?
Does the app
need to, itself, have 2 threads, one for execute_checkpoint() and
another one from
which it can issue the cancel_checkpoint() call?
I think having the app itself run execute_checkpoint() in a thread is
better. I thought about
having the execute_checkpoint() return immediately, and have the app
check for some sort of a variable to see whether execute_checkpoint is done,
but that doesn't sound very good. If the app
start the thread and run it, it can have better control.
I will add some discussion about this to the doc.
Also, I recall hearing before (and from Darren) that threading in
Python is considered
a bit flaky. Have you experienced this? If so, should be be listed as
a Risk?
I have not experienced with this specifically. I remember something
similar being
done in the Text Installer, so, I just assume that it will work.
6.6.1 - as this cleans up more than one checkpoint, should the name
not be
cleanup_checkpoints() (ie plural)?
Good point. Besides cleanup_checkpoint(), I think execute_checkpoints()
also
need to be plural, since we might execute more than one checkpoint.
Will change.
7.5 - the need for the force_doc_overwrite flag might change depending
on some
changes being proposed for the DOC, eg having separate
"non-snapshot-able"
(or volatile) and "snapshot-able" (or non-volatile) sections in the
cache. This
is still under discussion.
Understood. Will update the document as we make the final decision.
13.1 - as this is an important reference section, it might be good to
show the
args (and keyword params) for each method here?
Sounds good. Will add the args. I think I should add all the exception
too, since
those are also interfaces.
14 (diagram) - what do the messages labeled "chkp1", "chkp2" and
"chkp3" represent?
Is this importing the module or instantiating the checkpoint object?
If it's the
latter, then shouldn't these messages be going to the CheckpointObject
objects?
I am trying to show instantiating "chkp1" from "mod1", instantiating
"chkp2" from
"mod2". Instead of doing the way I am doing it now, I think I should
add a "getattr()"
message to get the "chkp1" class from "mod1", and then, send the instantiate
message to the checkpoint object.
Also, I'm by no means an expect on sequence diagrams, but this diagram
shows
the lifeline for all the objects existing from the beginning (ie the
rectangular
boxes are shown at the top of the diagram) rather than showing them
coming
into existence during the lifeline of the Application, which I think
is more
correct here. Does this make sense?
I am completely a newbie to sequence diagrams. I just learned enough of
it to do this diagram. :-)
You are completely right that it makes a lot of sense to have the
objects exist when they should. Let me look
into how to do that and fix the picture.
Also, a few typos, etc that might affect readability of the doc:
1 - first sentence contains some errors:
"the an interactive" -> "an interactive"
"and interactive" -> "an interactive"
"for for" -> "for"
4 (item 1.) - "quiet" -> "quite"
6.3.4 - "impl.find_module" -> "imp.find_module"
7.1.1 - (2nd para) - "running the all" -> "running all the"
7.2 - "feature to for" -> "feature for"
7.3.2.2 - "termites" -> "terminates" ;)
7.5 (Input section, 5th bullet) - "false" -> "False"
14 - "using many engine provided ..." Not sure what this is meant to be:
"engine" -> "features" ???
15.4 (last bullet) - "dataset s well as" -> "dataset as well as"
"DataCacheSnapshot" -> "DataObjectCache snapshot"
Fixed all the typos.
Thank you so much again for taking the time to review.
--Karen
- Dermot
Tung wrote:
Hi,
The draft install engine design doc is ready for review.
I pushed the doc to the caiman-docs repo. You can
find them under the install_engine directory in
ssh://[email protected]/hg/caiman/caiman-docs
If you don't want to clone the gate, you can also access
the file directly here:
http://cvs.opensolaris.org/source/xref/caiman/caiman-docs/install_engine
You will find 2 files in the directory:
* engine-design-doc.odt is the design document
* engine-sequence-diag-0528.png is the sequence diagram inserted into
chapter 14 of the document. You might want to see the bigger image
directly.
Please send your feedback by 6/11/2010.
If you plan to review the document, please send me an email privately,
and preferably also let me know when you will complete the review, so
I can plan accordingly and ensure the document is reviewed thoroughly.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
