Hi Jean, Please see my follow up questions/comments inline. I removed items that I have no further comments on.
On 02/23/10 02:44 PM, jeanm wrote: > Karen, > > Thanks for your comments. Responses are inline. > > Jean > > On 02/22/10 05:47 PM, Karen Tung wrote: >> Hi Jean, >> >> Here are my comments: >> >> General Comments: >> --------------------------------- >> >> - In each of the object types for the 3 different transfer types >> discussed in the >> document, there are attributes that will get used by every single >> operation within that object type, like the "destination directory". >> There are attributes >> that will get used only by certain operations ONCE. For those >> attributes, I don't >> think they need to be kept as attributes for the object at all. They >> can just >> be passed in as arguments when a certain operation is called. I >> think the "transfer()" method >> for IPS will be much cleaner if we adopt this philosophy. > For the IPS and SVR4 transfers it looks to me (with the new table you > requested) that the attributes are used > in several places. For the Media transfer that isn't necessarily true. > In fact, the file list, dir list and > skip file lists could all be passed in on the call to transfer. That > would be a nicer interface > than setting them through a method. After seeing Sarah's comment about how the transfer module should get all the parameters it need from the data cache, and Dave and Shawn's comment about how the current proposed design is duplicating existing IPS API functionalities, I wonder whether any of these functions or parameters is needed. Furthermore, will the transfer module be a checkpoint that's registered into the engine and run by the engine, or will it provide functionalities to do "transfer", so, a checkpoint can be implementing using those functionalities? > >> >> - There's no discussion about how, if any progress will be reported >> back to the caller of >> the transfer module. Does the caller pass in a callback function? > I'll be using the logging module that Sanjay and Ginnie are in the > process of designing. > I've talked with Sanjay and there shouldn't be a callback function but > there will probably > need to be an additional attribute to hold the logging object. I'll > add that into the spec. Not sure whether you need to add an attribute to hold the logging object. You can always get the logging handle from the engine. After I flush out the details on how to get it from the engine, you can include that info. >> >> - There's no discussion about how to interrupt a >> transfer-in-progress. This functionality >> is required when people choose to quit the installer in the middle of >> a transfer, which could >> be a long process. > You're right. I'm not sure exactly what section that belongs in, > overview??? I believe we would want > to catch the KeyboardInterrupt and exit. I don't believe catch keyboard interrupt and exit should be done in the transfer module. Catching the keyboard interrupt and determining what to do should be part of the application's job. What I am talking about is an explicit function that the engine can call to immediately stop the transfer-in-progress. The "tm_abort_transfer()" function in the existing transfer module provides the kind of functionality I am talking about. > > >> >> Specific Comments: >> ---------------------------------- >> >> section 2: >> - "The checkpointing module will be available" is one of the >> dependencies. >> I do not see how checkpointing is being used in the transfer module. >> Can you >> elaborate when and where it will be used? > > My understanding is that each module would be responsible for calling > the checkpointing functionality. > So in order to obtain checkpointing functionality, the transfer module > will need to call into it when performing > the transfer. I envision this as being part of the __init__ method. My understanding is that the engine that would be responsible for doing the needed checkpointing. The checkpoints don't need to worry about that. > >> >> section 3.4.2: >> - What about mirrors for the publishers, how do they get specified? > > I was thinking of that as being one of the ips_specific_args but I > could also add an attribute > to specify this if you think that is necessary. when I think about ips_specific_args, I was thinking more about those -v, -p...etc.. Since you have explicit calls for setting publishers and alternate publishers, perhaps mirror can be an additional argument to those functions since mirrors must be associated with a publisher. > >> >> section 3.4.2.2: >> - There is no "destination directory" for the IPSTransfer object. >> That should >> be a required attribute, right? > I named it alternate root, see below. I think Sarah already mentioned it in her comment. I think it would be better to name all the "destination" directories the same way. >> >> section 3.4.2.2.5: >> - Why we need an alternate root. Isn't that the "destination" >> directory? If so, I think calling it alternate root is confusing. >> Furthermore, this >> should be a required attribute. Default value of "none" won't work. > Actually it's not required. This is equivalent to the -R option when > you run pkg. If it's not > present, then the operation is done on the image discovered > automatically. What do you mean "image discovered automatically"? > > I'm not sure which naming is more confusing because it really is an > alternate image rooted at <dir> > that you want to operate on. If others comment on this name I'll > change it. > > >> section 3.4.3.2.1: >> - While we can map all SVR4 package operations, I don't understand >> how "INFO, PARAM, and TRANSLATE" >> can be useful in the transfer module. > > They are provided in an effort to give the consumer that has access to > only SVR4 packages full functionality. > They may need information about packages that pkginfo and pkgparam > provides. It's also not clear that they > might not want to translate to from one package format to another. Unless we have a valid use case for how we can use this, IMO, we should not include it. > >> section 4: >> - Since you have all the APIs defined, it would be useful to actually >> write some psudo-code for the >> use cases, instead of just describing what each of the installers are >> doing. >> That way, it would be easier to see how the APIs can be used. > > I've never actually seen pseudo-code in a design spec so I'm not sure > it's appropriate. If so, I think it > would go better in the class specific sections rather than the use cases. I guess I was just looking for some examples on how to use the new APIs. Perhaps not in the use case sections, but it would be good to show how to use them. Thanks, --Karen
