Hi Dan, Thanks for your comments.
On Wed, Jun 24, 2009 at 04:47:08PM -0700, Dan Price wrote: > On Tue 23 Jun 2009 at 01:34PM, [email protected] wrote: > > The following webrev converts our transport from using the > > urllib/httplib in python to PyCurl, which is based upon libcurl. > > > > http://cr.opensolaris.org/~johansen/webrev-xport-1/ > > > > Comments welcome. > > -- > Meta-comments/questions > > Now that 'nasty' is (going) in the gate, would it make sense to have > a "stress" section in the test suite which somehow used nasty mode? > > Also-- I could run ipkg.sfbay/nasty, a front for dev/ with a different, > nasty personality. At least for my testing, I still found myself fiddling with the nasty value, and changing the frequency of various failure cases. My expectation is that developers may want to poke at this a bit, depending upon what they're testing. It might be better not to run this on ipkg. > -- > client.py:2443 -- would be really nice if this snippet could > move into the transport layer (or somewhere common). I noted > that it is repeated in the GUI. Probably doesn't exist > at all in the update manager... Perhaps, except that we need to set the socket defaulttimeout early on in the clients. The rest is probably safe to keep in the transport. Would writing this as a common method in misc.py work better? > -- > fileobj.py: I'm not sure the comment at 223 is right-- it > talks about curl, but this object seems to not know > anything about curl. Changed to engine.run() > Also, I didn't really understand the header processing, > as I mentioned to you offline... I finally figured out why it was there. The updatelog looks at the headers to determine what type of catalog we were sent. It's the one that calls getheader(), although any consumer that needs to look at transport headers could do this. > -- > pkgplan.py: I don't totally get this programming model-- > why would mfile be None after the call to multi_file? And > wouldn't we want that to be an error of some sort? Or, > should it be something like: > > mfile = self.image.transport.multi_file(...) > if mfile is not None: > do stuff > self.__progtrack.download_end_pkg() If we're uninstalling a package, the plan may not have a destination_fmri. In that case, we don't have any files to download and it's safe for multi_file() to return None. If we do have a destination FMRI, then we want to download the files. I'll modify the download function to return early if mfile is None, since we shouldn't need to iterate through the actions if we're not going to download anything. > -- > modules/client/transport/stats.py: > > Perhaps dump should just wrap around __str__() for this object, > which would format stuff as you like? Might be more flexible > for future expansion. Can you clarify whether you mean that __str__() should do the formatting, or that dump() should continue to do the formatting, but it should get the data as a string from __str__()? If it's the latter, I'm not certain I understand how to do this. I didn't think there was much opportunity for further formatting once __str__ returned us a string? > -- > modules/server/catalog.py: > > You might want to add a comment that we're rolling the dice > twice here-- first, to see if we're going to be nasty at all, > and then again later on specific lines. Sure, I've added a comment after the first # NASTY > -- > modules/server/repository.py: > > Not sure how NastyRepository:catalog is different from its > superclass's implementation. I might have missed it somehow? The difference is subtle. Shawn asked me to include a comment about this, so I've added a docstring to the class that outlines the difference. Essentially, I'm passing scfg to the Catalog and UpdateLog (nasty versions) so they can roll the dice to decide when to be mean. > -- > SUNWpython-pycurl/Makefile: > > 31-32-- I know you copied this from somewhere, but please > nuke this comment. Nuked, as requested. > -- > setup.py: why is PCHASH none? Forgetfulness, I'm sure. I've added a SHA-1 hash for the 7.19.0 version. > -- > t_api_search.py: I take it 'sleep(1)' is to ensure unique > timestamps? Yes, apparently my new workstation is so fast, that it can send the packages before time advances. Pretty neat, eh? Thanks again for your feedback, -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
