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.
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"
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.
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)")
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?
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?
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.
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?
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.
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?
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)?
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?
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?
6.6.1 - as this cleans up more than one checkpoint, should the name not be
cleanup_checkpoints() (ie plural)?
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.
13.1 - as this is an important reference section, it might be good to
show the
args (and keyword params) for each method here?
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?
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?
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"
- 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
