On 08/ 2/10 10:55 AM, Keith Mitchell wrote:
Hi Jean,
I mostly have some clarifying questions, with a few comments/fixed
typos thrown in for good measure. Sorry for the delay - there's been
quite a bit to review lately!
Thanks,
Keith
4.1:
1st paragraph: The statement that the "Solaris" partition id is no
longer used because it collides with Solaris2 is only partially
correct, isn't it?
Yes. Solaris could be discovered if there is an old disk.
What I mean is, even though the new OS our installer puts down will
definitely create a Solaris2 partition, for compatibility reasons the
data structures need to understand that an existing LinuxSwap
partition could actually have Solaris slices on it, which means it
can't easily coexist with a Solaris2 partition. The data classes need
to be aware of this on some level.
Yes. We need to assume that partition id 0x82 is Linux until proven
otherwise, and there can only be one Solaris partition/disk whether that
is Solaris or Solaris 2. We'll add this to the doc.
Diagram: The text says the "VTOC label is not shown," but slices are
listed under the Solaris partition.
Removed.
6.1: Text implies that only __setitem__ and insert() are overridden,
but the diagram seems to indicate that several other functions are
overridden.
The diagram is correct, text is incorrect and will be fixed. However
subclasses of shadowList only have to override the two methods listed
(__setitem__ and insert). The text will be updated to state this also.
Can you expand on what the ShadowList will be doing? From prior
discussions, I'm guessing it will be asserting that all items in a
list are of the same type, but it's not clear from the usage here if
that's the case.
The ShadowList provides the common implementations for everything in the
diagram that is not __setitem__() and insert().
Will add text about this.
6.2:
Diagram 1: HolySlice vs. HoleyPartition - 'e' or no 'e' in Holy? What
do those objects represent?
There should be an e. They represent unused space between partitions or
slices so the application doesn't have to figure this out on it's own.
Nit: I don't think disks have an "offset," do they?
No. But all PhysicalTargets have an offset so we make Disk have an
offset property that always returns 0.
Nit: Partition.part_type/part_id -> could these just be "type" and
"id"? As they are attributes of a class, they shouldn't end up
clashing with the Python namespace builtins of "type()" and "id()".
Yes they could be and probably will be.
Diagram 2:
Can you explain the "Mixins"? I know I've read about this Python
concept before, but it's still not concrete, and there's no
explanation of the Partition/SliceChildrenMixin classes here.
We'll explain with an example. Both DiskPartitionChildren and
PartitionExtChildren inherit from PartitionChildrenMixin.
When you add a partition to a disk it's similar to adding a logical
partition to an extended partition so we're putting the common code in
the mixin. It could just be a function that they just take advantage of
instead of a mixin and might end up that way in the long run.
I'm not certain all the subclasses will be required explicitly. For
example, where the only thing changing is basic values like the
"limit," once could simply instantiate different objects and have a
limit parameter, rather than an entire class for it. I've not thought
about it deeply, but I almost think a single PhysicalChildList class
could be sufficient, with appropriate parameters for limit, class(es)
accepted, etc.
The reason for having separate classes is so a disk doesn't have to
check various combinations of PhysicalChildList.
For example:
If you make a PhysicalChildList that only holds primary partitions and
is limited to 4. The Disk class needs to be able to tell that is a valid
PhysicalChildList. You could also have a PhysicalChildList that contains
Slices limited to 16.
The code in Disk now has to check for at least 2 possibilities:
if physlist.type = Slice:
assert(physlist.LIMIT = 16)
elif physlist.type = Partition
assert(physlist.LIMIT=4)
But with the class hierarchy there is just one test:
assert(isinstance(object, DiskChildren)
And that one simple test remains valid even if we add a DiskFooChildren
that inherits from DiskChildren.
Page 16:
The type() function has very limited value for making logical
decisions (mostly due to complications from subclasses). It'd be best
not to encourage its use in this fashion - isinstance() should work
fine here.
Agreed, type is limited and isinstance() is more useful. But in this
example where we set function and we use type, we need a type returned
and not a bool.
Side note: The classes specified here will all include __str__ method
definitions, correct?
Yes, or __repr__
7.1.2:
Can you clarify what other factors, if any, would affect the value
returned by get_progress_estimate()? e.g., zpool searching is
dependent on the number of the disks (which may already be known, if
disk searching was already done).
We'll think about that.
7.2: Nit: Last paragraph would be more helpful if listed first.
Sure.
7.3: I think it'd be better to have a separate error class for
"received libtd error" and "incorrect input" - suggest perhaps
ValueError for the latter. "DiscoveryError" would also be a more
descriptive name for TDError.
The ManifestParser design specified raising one error class that
contained a message and the original error object.
In order to make all the checkpoints consistent, we'd like to follow
that methodology. No issues renaming it to DiscoveryError instead of
TDError.
Will libtd still be in C? If so, in the last sentence here, "exception
raised" is misleading (error/return code would be more accurate, I
think). If not, I strongly caution against re-masking a Python
exception unless specific additional context can be added.
Yes libtd will be written in C. We will be interfacing with that library
using the ctypes module so it will be raising exceptions.
8.1.1: Nit: If the dry_run flag is set to True, then *no* changes to
the target will be performed
Thanks.
8.1.3: I'm not certain how much value there may or may not be in this,
but would it be possible to send a summary of what was
destroyed/created to the logs, if the install was canceled? It could
be convenient to have an idea of the current disk state, though
admittedly the amount of use that would get is very small.
Maybe. We'll take it into consideration.
8.2: So 'destroy' objects will simply be names like "c0t0d0s0", and
not full Physical/LogicalTargets? Would it be possible to accept both?
It will be a fully qualified path in the DOC, something like
discovered_target.children[4].children[3].
The idea is to try to make it easy to specify exactly what you want to
destroy and not have to specify a huge tree. You also don't want to
destroy anything that wasn't discovered so it ought to be in the DOC.
For "create," what would happen if the disk originally had a Solaris2
partition and a Windows partition, and the object under "create" was
that disk with just the Solaris2 partition (unchanged) with modified
slices? Would the Windows partition be destroyed, left untouched, or
would this be an error?
This is the exact reason we have the destroy object. What will happen
despends upon whether you tell us to destroy the Windows partition or not.
8.3: Same comments as 7.3
Same answer.
Jean & Dave
On 07/27/10 12:34 PM, jean.mccormack wrote:
Please review the design doc for the Common Object classes/TI/TD
design located at:
http://hub.opensolaris.org/bin/view/Project+caiman/CUE_docs
We'd like to get comments by COB on Tuesday August 3rd.
Thanks,
Jean, Dave, & Sanjay
_______________________________________________
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
