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. > 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
