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. > > - 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. > > - 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. > > 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. > > section 3.4: > - General comment for all the classes. You don't have a constructor > method > that talks about what options are required when the object is created. > I think it would be very useful to list all the required/optional > arguments > to the constructor method. Sure. I'll add those. > > section 3.4.1.2.3: > - The second sentence is not finished. You're right. Fixed. > > section 3.4.1.2.4: > - "Default value is None" is not quiet correct, I think. If _type is > FILELIST, this value > is required. If _type is not FILELIST, then, this value is not relevant. Yes it is required if _type is FILELIST. If the user doesn't specify it, then it's an error condition. My thinking was to make the default value None and if it's None when you run the transfer, then you kick out an error. > > section 3.4.1.2.5: > - Same comment as above for the "Default value is None" statement. Same response. > > 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. > - You kinda mentioned that IPS APIs will be used for implementing the > functionalities > in the object. It would be good to explicitly spell it out in this > section. Perhaps also > discuss why you choose to use IPS API instead of the pkg(5) commands. I already have it in the transfer method but I'll add it here along with the reasons. > > 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. > > 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. 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.2.3.1: > - It would be much easier to read if this information is in a table > format, I think. > - "PURGE-HISTORY will use....". Need to finish this sentence. > - "SEARCH will use the _args....". I am not sure what we are > searching for here. Package names? > Files inside packages? You're right. Good idea. I'll change it to some type of table. Search can be used to search the pkg database for your query value. > > 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. > > section 3.4.3.2.2: > - Again, do you mean "destination directory" here? Also, default > value can not be None. This is equivalent to the -R root_path argument in pkgadd. Since that is a optional argument it is not required. It's really meant as an alternate_root thus the name. > > 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.
