Joe, Thanks for the feedback. Response is inline.
Jean On 02/26/10 10:35 AM, Joseph J VLcek wrote: > On 02/22/10 04: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 > > > Looks great Jean! > > I mostly have questions and a couple of nits... > > Joe > > - - - > > General Comment/Question: > ------------------------- > > Will an IPS repo on media need special treatment or will the > Transfer Module be able to treat it as an HTTP: IPS repo? If it's available as an IPS repo then the transfer module should treat it as such. > > General Comment/Question: > ------------------------- > > Would it be possible for the user of the Transfer Module > not have to know the transfer type, media, file, DVD, > IPS HTTP repo...? > Not really. There are way too many differences between the different types of transfers for the user not to have to know. > Would it be better to try to provide a more general API > which could be used by all? Perhaps different fields/member > functions would need to be used. Can you explain what you're thinking here? > > Forcing the user to need use a different class depending > on the transfer mechanism, MediaTransfer, IPSTransfer, > <future>Transfer might not be the best API from the users > perspective. It will actually be a subclass and since the transfers are so different, I really don't see a way around this. The application level could be ignorant of the transfer type (maybe?), but the Transfer Module checkpoint code will really need to know via some means. > > > > Comment: > -------- > > Under "1 Purpose" > > - various different transfer protocols. > > Are these really "protocols"? "Mechanisms" seems to me to convey this > better. > done. > Comment: > -------- > > Under "1 Purpose" > > > like do_clobber ... will not be implemented. > > Where will that functionality be implemented? I need to discuss the where with Sarah. > > > Comment: > -------- > > Under "2 Assumptions and Dependencies > > The data source will exist and be specified by the caller. > > Will it's existence be validated? Yes. Validity of parameters is a must. > > Comment: > -------- > > Under "2 Assumptions and Dependencies > > The caller will know and specify the type of transfer required. > > Couldn't the Transfer Module determine the specify the type > based on the source? Maybe. The source between SVR4 and cpio might be hard to do. And if we extend the module to include other types of transfer that have similar sources it becomes risky. There's also the issue that some of the attributes that the different transfer mechanisms need are very transfer type dependent so you need to know anyway. > > > Comment: > -------- > > Under "3.1 Overview" > > Nit: This sentence seems awkward to me: > > The design needs to be easily extensible so that if a new transfer > type is desired, the module will be able to be modified using the > same interface methodology and without modifying the old interfaces. > > Suggestion: > > The design needs to be easily extensible to accommodate future, new, > transfer > mechanisms. > Much better. Thanks. > Comment: > -------- > > Under 3.3.1 "Exported Interfaces" > > > Nit/Question: Is "Media"Transfer the best term to use here? I realize > we have been using it but perhaps "file" transfer or something else ? > might better provide for future transfer mechanisms. Or CPIO Transfer? It's definitely more similar to the names for the IPS and SVR4 transfers. > > Comment: > -------- > > Under 3.3.1 "Exported Interfaces" > > > Could a MediaTransfer "rm_arg" prove useful? That's even more tricky than the add_args that Dave suggested I get rid of. The user can just get a rm by reading the args and then setting them with the offending arg removed. > > > Comment: > -------- > > Under 3.3.1 "Exported Interfaces" > > > Nit: > > IPSTransfer set_type is listed twice. Done. You're the 3rd to find that one! > > Comment: > -------- > > Under 3.3.1 "Exported Interfaces" > > > Could a IPSTransfer rm_arg(S), rm_from_pkg_list, prove useful? Nah. See above. > > Comment: > -------- > > Under 3.3.1 "Exported Interfaces" > > > Could a SVR4Transfer rm_arg(S) prove useful? Nah. See above. > > Comment: > -------- > > Under 3.4.1.2.1 "_dst_mntpt" > > Nit: > > "Must be specified" is repeated Fixed. > > Comment: > -------- > > Under 3.4.1.2.4 "_file_list > Under 3.4.1.2.5 "_file_list > Under 3.4.1.2.6 "_file_list > other places too... > > Nit: > > Default value is "" > > > Maybe be more specific e.g. the Default value is an empty list > or the null string. OK. > > > Comment: > -------- > > Under 3.4.1.2.6 _skip_file_list > > The description makes me think this should be _remove_file_list. > > Also why is this restricted to the ENTIRE mode? remove vs skip implied implementation was my only thought. Why is it restricted to the ENTIRE mode? Because if you're transferring a list of files in the first place, why would you remove some of them? Why just not include them in the list of files to transfer? > > > > Comment: > -------- > > Under 3.4.2.2.2 _alternate_publisher_uri > > Can the user supply multiple uris? Yes. > > > Then maybe _alternate_publisher_uri_list might be good. Yeah that would work, but just gets kind of long and since it's defined as a list.... > > Comment: > -------- > > In the table on page 12 what form with LIST produce? I'm thinking a list of tuples. > Could these list get long? yes. > If so would list access > class members be helpful to the user? Possibly. > Get next element... kind of thing? That's interesting. I'm going to defer a bit on this until the debate about IPS functionality is resolved. > > Comment: > -------- > > Nit: P 20 provies spelled wrong. > > The Automated Installer provies an > > "th esystem" spelled wrong > > vice that th esystem fixed. Jean
