Hi Darren,
Thank you very much for reviewing the document. My responses are inline.
On 06/09/10 02:24, Darren Kenny wrote:
Hi Karen,
The document looks good - but as I'm sure you expect there are some comments...
I've not mentioned anything specific to the use of the DOC for checkpoints, I
think that we've enough of a discussion going on in the other threads on
this... :)
Comments:
Section 3, page 5 :
DataObjectCache should really read "Data Object Cache" to refer to the
overall project rather than the specific class...
OK. Will fix it there. I will also look through the whole document to
make sure
I use the correct terms.
Section 4, page 5:
I'm wondering if the programming language to use is really a choice for
this project since it's been decided at the higher level of the CUD design
that Python is the language of choice for new implementations.
I didn't see any specific mention of Python in the architecture design doc.
It only talks about using an Object Oriented language. So, I thought I will
be complete, and talk about other potential choices here.
Section 5.2, page 6: (and in some way Sections 6.5 and 10)
The "get_progress_estimate()" method could do with some definition of what
should be returned here - is it a time estimate (seconds or minutes)? I
certainly think that it would be useful to have an example of how this
should be used by developers - I realise section 10 provides some idea of
what this is, but probably useful to spell out an example, and probably
best done in section 10 to also provide an example of normalizing.
I think I will give more description on each of the required
to implement methods, and make a reference to section 10
about what type of value to return. From the comments I received,
looks like everybody want more information about what kind of
value should be returned for progress, and how that value is
determined., I will try to have more details in section 10.
The provision of a "cancel()" method implies that things are
multi-threaded - I'm wondering at what level threading is being done, is
it at the Application level, the Engine level, or the Checkpoint level.
If threading is done by the Engine - i.e. all checkpoints are run in a
thread, one after the other, does this imply that the "execute()" method
is asynchronous, in that it will return immediately and not wait for
execution of the checkpoints to be completed - if this is the case a
notification mechanism to let you know when things are totally completed
would be useful - I know progress kinda gives you this, but it would be no
harm to have a "completed" call-back.
Section 6.4.2 talks about the fact that the execution is
done in a separate thread. In the 2nd paragraph, it explicitly
talks about the fact that the execute() function will not return
until after all checkpoints are executed successfully or one of
the checkpoint failed. So, we don't need a "completed" call-back.
Relying on progress monitoring might not be a good idea, because
if the application didn't register progress monitor with the logger,
or if the checkpoints don't report their progress to the logger, the
application
won't know the progress.
If the application want to do something else when execute_checkpoint() is
running, it should run execute_checkpoint() in a thread.
I think a section on threading would be useful, since I believe it has an
impact on other elements of the install infrastructure.
Good idea. Let me add a section that talks about this.
Section 6.1, page 7:
The DOC is not a singleton any more, and there will be a need of a
"get_data_object_cache()" method on the Engine.
Do you think we need to define a function that just returns the variable?
We can allow people to access the doc variable directly, and make the
name of the variable an interface. We can use the "property" feature
in Python if we found that we need to define a function later.
Section 6.3.1, page 8:
Maybe the use of the term "dangerous" is too severe? It might be better to
be specific about the risk, and how it might be able to be prevented.
I can talk about risk, but is there really a way to prevent it?
We do allow checkpoints to exist everywhere. Do we want to restrict it
to loading from a certain location?
Nit: last paragraph : "The user is allow to" -> "The user is allowed to"
Fixed.
Section 6.3.2, page 8 (and some others like Section 7.4.1)
Nit: The references to checkpoints "a, b, c..." - it would be useful to
somehow make these stand out from the rest of the text (capitalize, bold,
italics, etc - or use a specific style like "Source Text").
Good idea. Will change.
Section 6.3.4, page 8:
register_checkpoint() parameter "checkpoint_obj", would be more correctly
known as "checkpoint_class_name".
Changed as suggested.
Section 6.7.1, page 12:
How are you going to guarantee that removal of snapshots from /tmp will
not remove snapshots from other processes - e.g. are you using a dir for
each run (/tmp/install_snapshots.PID/), just might be worth spelling out.
Section 7.4, the second bullet point did talk about using PID for DOC
snapshots that are stored in /tmp. Do you think that's sufficient?
Section 7.3.2.2, page 14:
Nit: s/termites/terminates/
Fixed.
Section 7.5, page 15& 16:
How is "DataObjectCacheNotEmpty" state determined? Should the DOC provide
a is_empty() method is or just checking that len(doc.children) == 0,
enough?
I was going to use doc.get_descendants() to make sure it return
an empty list. Looks like I can use len(doc.children) too.
Are they both the same? Is one better than the other?
Thank you again for your review.
--Karen
Thanks,
Darren.
On 05/28/10 11:12 PM, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
