Sarah Jelinek wrote: > Hi Sue, > > Thank you for reviewing this. Comments inline.. > > >> Hi Sarah, >> >> Here are my comments. >> >> 1.0 Does the distinction between 2,3 and 4,5 mean that one should be >> able to boot from one type of environment (network, media) and >> install from another? > > Yes, basically that's what this means. For example, you could boot the > text installer from local media, but install from an IPS repository. > > >> For 10, I assume you mean the first boot after the installation is >> complete? If so, adding those words would make it clearer. > > Yes, that's what I mean. I will modify this. You are the 2nd person to > note that this sentence is confusing. > > >> The term "this project" or "the project" is used in various places >> (first referenced towards the bottom of p2). What does that term mean >> wrt this document? Everything under the Caiman umbrella? Just the >> implementation of the architecture (core engine and common app classes)? > > In this document it refers to just the components of this document. > But, the use of 'the project' implies it is one project and it isn't. > Let me think about how to refer to this work in a way that is clearer. > > >> 2.1 "The engine is capable of beginning execution at the next >> unreached checkpoint". So if one has reached and stopped at >> checkpoint 1, they can continue from checkpoint 2?? > > If one has reached and successfully run checkpoint 1, then they can > start at checkpoint 2. >
Now both Sue and I have commented on this so I don't feel so bad poking because it means I'm not really that dense. How does this work? There are 2 ways that I see checkpoints could work 1) you checkpoint before functionality for the checkpoint (like DC does it) or 2) you checkpoint after functionality for that checkpoint. Case 1: You run checkpoint 1. That means functionality between chkpt 1 and chkpt 2 has not been run so how can you "begin" from 2? picture: ----> chkpt 1 some functionality for step 1 ----> chkpt 2 some functionality for step 2 You will now miss running the functionality for step 1 Case 2: You run checkpoint 1. That means you have run the functionality associated with chkpt1 but not yet 2. Since chkpt 2 is after that functionality again you miss something. picture: functionality for step 1 ----> chkpt 1 functionality for step 2 -----> chkpt 2 In this case you're missing the functionality for step 2 Are you considering some kind of funky overlap? Like the following picture? ---> starting checkpoint for step 1 functionality for step 1 ---> completion checkpoint for step 1 ----> starting checkpoint for step 2 functionality for step 2 ---> completion checkpoint for step 2 So you would be aliasing end checkpoint for step n = start checkpoint for n+1? Jean >> remove_logger: from execution->from use during execution >> Similar for remove_progress_receiver >> > > Will change. > > >> 2.2 >> o to_xml(): why did you choose xml? what else was considered? > > this will be going away. Even if the internal store is XML, we won't > be exposing that to the consumers. We haven't settled on the format of > the cache and it doesn't matter what it is anyway. > >> o retrieve_data(): (nit) methods->method > > Fixed. > >> o dump_cache(), "contained in the cache in an AI manifest format". >> Maybe add a reference or footnote here for the reader who doesn't >> know what this format is? > > Sure, good idea. I will add. > > >> 2.3 Is the implementation of the architecture assumed to be in >> Python? This section refers to Python Logging module)? If so, perhaps >> you should explicitly state that up front somewhere. > > No, it hasn't been decided if the whole architecture will be > implemented in python. Sundar pointed out that this implementation > detail doesn't really belong in the arch doc, and he is right. I will > remove this. > > >> 3 Mentions AI and DC. Should there be references here? > > Yes. I will add. > > >> 3.1 >> o add comma after: "After parsing and validating the manifest" >> o under parse(): will do-> does > > Fixed. > >> 3.2 >> o Second paragraph, maybe add "in the case of x86" before disk >> partitions > > ok. > >> o (nit) passed in to the objects -> passed in to the object's > > Fixed. >> o The See section 3.2 should reference 3.2.1.1 and 3.2.1.2, otherwise >> points to itself. > > Fixed. > >> o Not clear to me why the seed is needed? > > Target discovery can be done on my types of targets. For example, with > installation support for both S10 and S Next in an existing zpool, we > need to discover the existing zpool, not the devices such as disks. > So, the 'seed' value that is passed in to the TargetDiscovery class > tells it what type of discovery we need it to do. We want to make this > class useful to all Caiman applications that require target discovery. > > >> 3.2.1.1 >> o Disk Class - there are two get_assoc_slices(). One should be a >> set_assoc_slices(). > > Fixed. > >> o Partition Class - should mention x86 only > ok. >> o Slice Class, what does get_flags do? What kind of flags? Maybe >> change name to be more descriptive? >> > > Probably needs to be more descriptive. It is really, in this doc, just > a way to show we have to worry about slice flags, such as ROOT, or > USR, settings, but the details will go into the design document for > the data modeling work. > >> 3.2.1.2 >> o Logical devices - It seems odd that there is an instantiate method >> under TD. There are also several set methods which may fall into the >> same category. Do these classes have methods for both TD and TI? If >> so, the document should be more clear on that point. > > Yes, they have methods for both TD and TI, since we want to reuse the > classes. I can make this clearer. The 'instantiate()' method will > likely be renamed to ensure that it is clear what it is being used for. > >> o Dataset Class - represents a ZFS datasets -> represents a ZFS dataset > > Fixed. > >> 4.1 You point to the secure wiki for a detailed class diagram. Since >> this will be an open document, shouldn't the class digram be moved to >> osol so external users can see it? > > I will move these to opensolaris. > >> >> 4.2.1 I am seeing: "The following diagram Error: Reference source not >> found, broken into multiple...". > Yeah.. I saw that too. Not sure why. I will fix. > >> 4.2.2 The 4.2 in "Drawing 4.2 shows the sequence of Target Discovery" >> should be 4.3 > > Fixed. > > Thanks again for the review. > > sarah > **** >> >> Thanks, >> Sue > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
