Hi Sue,
Thank you very much for reviewing the document. My responses are inline.
On 06/ 9/10 11:33 AM, Sue Sohn wrote:
On 05/28/10 15:12, Karen 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
Hi Karen,
Here are my comments:
General:
o Nice job on this document
o The Table of Contents does not match the document. For instance the
TOC lists section 9 as DataObjectCache interaction, but the document
has section 9 as Logging.
Hmmm...don't know why that happened. I assume the TOC will get updated
to match the content of the document as it is being updated. Looks like
it does not. I will fix that.
o Sometimes you use "Data Object Cache" and sometimes DataObjectCache.
Is there a distinction?
I thought they are the same, so, I use them interchangeably. But Darren
pointed out that
I should use Data Object Cache for the project and DataObjectCache for
the actual object
itself. I have gone through the whole document based on his
recommendation and made
all the corrections.
o Maybe add the word "Install" to the title - Caiman Install Execution
Engine
Sounds good. Will add.
6.3 "Information regarding the registered checkpoints are not stored
in the DataObjectCache until a checkpoint is successfully executed"
Can you be more specific about which kind of information here and why
the decision not to include it in the DOC until executed?
All registration information will be stored in the DOC. I will add the
clarification here, and also,
make a forward reference to how the information will be used in
stop/resume implementation
in section 7.4.1.
6.3.4 If loglevel is not valid for the log service, is exception raised?
Yes, an exception should be raised. The "ValueError" exception.
I will add that.
6.4 Is calling get_progress_estimate part of initialization? Second
paragraph implies not, that it is separate (first initializes, then
calls get_progress_estimate, finally executes), but 6.4.1 seems to
include it as part of initialization.
get_progress_estimate() will be called after the object is
instantiated. In 6.4.1, I want to talk about all the *things* that will
be done before checkpoints are executed. I think the current title
for 6.4.1 is not good. I think I will rename it to: "Preparing for
checkpoint execution".
6.4.2 If a checkpoint execution fails, what information is
communicated back to the application so that it knows where the
failure was?
The exception that is raised by the checkpoint will be raised to the
application. So, the application
should have the information on details of the error.
Your comment brought up a good point about how would the application
know which checkpoint
failed. I didn't have this in the design. For checkpoint failures, I
think the engine will define
an exception called CheckpointExecutionFailure, and then, it will have 2
attributes. 1 attribute
is the name of the checkpoint that fail and the other attribute is the
un-modified exception
from the checkpoint.
6.4.3 Instead of "pause_at", maybe "pause_before" to be clearer
(similar to register_checkpoint's "insert_before"). Also, the
description for pause_at says that execution will continue until all
registered checkpoints are executed. Maybe add "or until one fails".
Speaking of which, if a
Changed to "pause_before". Dermot made a similar suggestion too.
checkpoint fails, what happens? Is an exception thrown?
Yes, an exception will be thrown. See above for my discussion on what
exception is thrown.
6.5 Does the engine log anything if application has requested
cancellation?
Yes, the engine will log that.
7.4 It says when the engine terminates, all DOC snapshots in /tmp will
be removed unless debug is set. Why not copy them to the installation
target?
In general, we will only store DOC snapshots for checkpoints that have
coorsponding ZFS snapshots.
If we name the DOC snapshots based on the checkpoint name, I think we
can move all
the DOC snapshots from /tmp to the install target as soon as it becomes
available, and we can
support resuming from those checkpoints as well.
7.5 Is there an exception if both zfs_dataset and doc_snapshot are
provided (since you specify that only one can be provided)?
Yes, an exception will be thrown. ValueError exception will be added to
the exception list for such a case.
10.1 Is the progress estimation weight in seconds? minutes?
How/when will the TBD about generation be resolved?
The progress estimation weight will be a value that is derived based on
the amount
of time it takes to execute a checkpoint based on some formula.
I don't have any immediate plan to create the "formula" that would translate
time to weight yet. For now, I supposed we can use some guess "weights".
See my reply to Dave about this for slightly more details.
13.1 Interface table:
o is missing normalize_progress()
OK. Added.
o is it cleanup_checkpoint or cleanup_checkpoints (with an 's')?
You have both in several places in the doc, you should make it
consistent.
Should be cleanup_checkpoints() since the engine will clean up DOC and ZFS
snapshot associated with all the checkpoints.
I will make them all consistent.
o Should the functions in AbstractCheckpoint be here? I know
they're sort of a different category, but it seems at least close to
an interface that's exported.
Yes, they should. I will add them.
nit/typos (tried not to repeat what Dermot mentioned)
1 python based object to the drive image build process -> python
based object to drive the image build process
2 setups up -> sets up
3 (third bullet) initialize->initializes
4 (item 2) complied -> compiled
5.1 the application provide a -> the application provides a
an unique -> a unique
6.3 Application -> The application
6.4 first initialize -> first initializes
7.3.1 For an application that do not terminate, it can resume -> An
application that does not terminate can resume
7.3.2.1 For an applications -> For an application
7.5 First bullet under Input: resume at to -> resume at
9, 10. 10.2, 15.5 it's -> its
Fixed all of the above.
Thank you very much again for reviewing the doc.
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
