Hi Jean,
I reviewed version 2.0 of the document.
My first general comment - since there are certain functions that are
common across all transfer types, I think there may be value in defining
a superclass and moving some of the functionality there. Subclasses can
then implement specific attributes and methods they need in addition to
the common ones. The parent class could also weakly restrict values such
as "data source", whereas the subclasses could enforce that the source
is appropriate (e.g., an IPS repo for the IPSTransfer class).
Secondly, I wonder if the goal of the transfer module is possibly too
wide in scope? The direction of the Media/IPS/SVR4Transfer classes seems
to suggest we're simply providing python interfaces for CLI commands (or
in the case of IPS, an existing Python API). It seems like a transfer
module should make setting up and executing a "transfer" a
straightforward task. In short, I suppose what I'm asking is - what
benefit does a consumer gain from instantiating these classes and
calling transfer this way, vs. writing their own script or using the
'subprocess' Python module to run CLI commands? This isn't mutually
exclusive with flexibility, though - optional arguments to __init__, for
example, could be defined for those consumers that *really* have
specific needs.
3.1: Final paragraph on KeyboardInterrupt: This seems a bit specific. I
think it would be better if the 'transfer()' method implemented a
'finally' clause - this could then handle other uncaught exceptions.
Additionally, there should be some method of killing the transfer
process outside of simply a KeyboardInterrupt - for example, if
"transfer()" is asynchronous, a corresponding "cancel()" method.
3.4.1: General: Default values for attributes (such as _file_list and
_dir_list) should be None (assuming a Python implementation),
particularly if they're required.
3.4.1.3.1: Nit: The keyword arguments shouldn't have a leading
underscore - public interfaces/attributes should start with a letter
instead (The corresponding attributes, such as self._dir_list, can keep
they're existing names).
3.4.1.3.3, others: Nit/additional data point: Define these as python
'properties' for pseudo attributes. Most "getter/setter" functions are
more pythonic when implemented as attributes. Access to these functions
can then occur via "MediaTransfer.type" instead of get_type/set_type (I
only bring this up at this stage as it's an interface kind of nit)
get_args/set_args (All transfer types) - these seem pretty specific,
requiring the caller to know a fair amount about the transfer they're
calling. How much of it can be extrapolated away? For CPIO, for example,
a set of booleans that indicate the different CLI flags, with
intelligent defaults, seems more applicable then providing a potentially
awkward "args" interface. Most consumers could leave the flags be; if
one needed to toggle a specific arg, they could.
3.4.2.2.1: It would be handy if the default could be dynamic; for
'release' installs, default to /release; for 'dev' installs, default to
'/dev'. This would resolve several RE type issues as well, I imagine.
3.4.2.2.1/2: preferred/alternate publisher seems to be an old concept; I
believe it's now more of a prioritized list, and much of the information
(about mirrors and such) can be gathered by querying the publisher URI.
3.4.2.2.4: I echo Dave's and Shawn's sentiments here on whether we need
to provide a full wrapper around the pkg API. It seems more like we
should be condensing and using the API, not mapping it directly across.
For example, I would imagine a "transfer()" call for IPSTransfer would
image-create, set publisher(s) appropriately, pkg install, and then
purge history. This events could be stored in a list or some such thing
of the class, a set of actions that the IPSTransfer would perform. The
list of actions would have an intelligent default, but the caller could
also modify the default, or pass in its own set of pkg API calls for
execution.
4: It'd be helpful (for me) to see some simple pseudo-code or examples
of what the caller of the transfer module will and will not have to do.
e.g., an example of what AI might do to run the transfer - would it be
something like:
ips_transfer = IPSTransfer(altroot, publisher, ...)
ips_transfer.verify() # Might verify that there's enough space to
perform the transfer,
# and that all the pkgs exist
ips_transfer.transfer()
Thanks,
Keith
On 02/22/10 01:32 PM, jeanm wrote:
>
> Please review the design document for the transfer module of the
> Caiman Unified Design project.
> It can be accessed via:
> http://hub.opensolaris.org/bin/preview/Project+caiman/CUE_docs/CUDTransferModule.odt
>
>
>
> I would appreciate feedback by COB Friday February 26th.
>
> Jean McCormack
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
