On Jun 23, 2009, at 3:34 PM, johan...@sun.com wrote:
Folks,
For a while, our transport system has not really been adequate to
address all of the goals for the pkg project. The python libraries
that
we have been using are capabable for basic functionality, but fall
flat
when we try to perform more complicated or performant network
operations.
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.
Awesome!
src/client.py:
line 2452: missing '.' at end
src/modules/catalog.py:
line 739: missing '.' at end
src/modules/client/api.py:
lines 165, 318: does this mean that if a user already has
everything cached in /var/pkg/download, etc. and we can't contact the
depot to refresh the catalog (which they didn't explicitly request)
that the install/update will fail?
line 608: won't a bogus value in PKG_DUMP_STATS, like "true" cause
a traceback here?
src/modules/client/api_errors.py:
lines 298, 309: missing an extra newline
src/modules/client/image.py:
line 54: nits: can you move this to where pkg.client.retrieve was
(keeping the ordering) and reindent the list to match?
line 730: missing '.' at end
line 1228: question, what is the difference between 'raise' and
'raise e'; i've seen both forms used
line 1309: the comment says zero, but the code says '= None'
line 1310: missing '.' at end
src/modules/client/imagestate.py:
Nice! This seems like a better home for intent. Thanks for moving
it.
src/modules/client/transport/engine.py:
lines 28-32: nit: can you asciibetise imports in all your files?
it makes it easier to update the imports later
line 105: nit: shouldn't it be pathname instead of filename if it
is expected to optionally include the path?
line 111: nit: s/progress tracker/ProgressTracker/ to make it
subtly clear that only ProgressTracker objects are supported. This
likely is in other places as well.
line 151: in the gui, the user might initiate another transport
action even after canceling, should the remove_handle, teardown, free
happen still (lines 218-220)? I know we have the transport.reset()
call in the prepare() part of the client.api, but I thought I would
ask. It just seemed odd that for raise_to_ex that we would cleanup
first, while cancel immediately returns instead of cleaning up.
Though perhaps you did that because cancel has to be immediate.
line 181: httplib.OK would be preferable I think to '200'
line 227: extra newline?
line 231: missing newline between paragraphs?
line 234: s/Url/urllist/ ?
line 235: s/URL/URLs/?
line 240: s/Permanant/Permanent/
lines 265, 303: extra newline?
line 304: nit: since this is more like a read-only property than a
function, i'd decorate this with @property, and then callers can
simply say 'if engine.pending', but that's a matter of taste.
line 323: a comment explaining what the failure here is would be nice
lines 339-343: nit: can you re-indent this?
line 404: should this function validate that size >= 0?
line 467: nit: s/. /. /
line 530: extra newline?
line 532: s/cleaup/cleanup/
line 554: if you made this 'if hdl.success: continue' you could de-
indent the block below and the formatting would be nicer
lines 568, 620: missing extra newline
line 641: s/examning/examining/
src/modules/client/transport/exception.py:
line 76: missing extra newline?
line 78: extra spaces at the start and end
lines 153, 210: s/Raise/Used/ or s/Raise/Raised/ for consistency?
lines 168, 203: indentation is off four spaces
line 281: missing extra newline
src/modules/client/transport/fileobj.py:
lines 31-34: re-indent?
line 60: should this have progtrack so the function signature
matches for subclasses?
lines 154-155: I think it would be helpful to be clearer that
you're talking about the total size of all lines and not just
individual ones. I half-expected 'sizehint' to be passed to
readline() directly.
line 247: you hate it that much?
line 254: either ... ?
line 270: s/into/into a/ ?
line 278: s/transport/the transport/ ?
src/modules/client/transport/repo.py:
line 80: s/install/operation/
line 117: does this mean that clients of the transport engine
aren't intended to specify the versions of operations they want to
perform?
lines 153, 166, 180: why not just return self._fetch_url ... ?
line 239: missing trailing '/' for 'versions/0' that you seem to
use everywhere else
line 256: why assign to respdata? not used
line 316: should this check to see if the engine object id() is the
same or does it matter?
src/modules/client/transport/transport.py:
nit: pkg.client.transport.transport.Transport seems a little
redundant, shouldn't this be in pkg.client.transport.__init__.py so
that you have pkg.client.transport.Transport instead? Or does the
magic you have in __init__.py do that?
line 88: s/Return/Returns/?
line 130: docstring?
line 144: extra newline
line 153: use pub.meta_root instead
line 223: mcontent?
lines 259, 331: extra newline
line 434: right now, rename is fine because we aren't going cross-
device, but in the future, if the cache directory path is customisable
(?), or we separated things out into separate zfs filesystems, this
may become an issue
line 579: s/operations/operation/ ?
line 606: missing '.' at end
line 688: extra newline
line 789: s/Make openers/make_openers/
line 791: s/make openers/make_openers/
src/modules/misc.py:
line 129: extra newline
src/modules/server/catalog.py:
line 390: sounds nasty
lines 377-385: is this override actually needed? I thought
__init__ got inherited
src/modules/server/config.py:
line 299: extra whitespace after '='
src/modules/server/depot.py:
You had way too much fun writing this.
src/modules/server/repository.py:
lines 339-380: Can you mark the differences between this and the
normal catalog method somehow?
src/patch/*:
It would be nice to see some comments at the beginning of each
patch file with a URL to the corresponding bug number of a brief
explanation of its purpose.
src/tests/*:
Are there any unit tests missing or did you feel like the client
just using the depot is enough?
I also tried out an image-update from 111b -> 116 locally against my
own depot server with the cherrypy 408 timeout patch. From what I can
see, it doesn't stall nearly as much when downloading like the old
transport layer, and makes steady progress.
Even though I know the new transport can be 'slower' overall, it
definitely 'felt' faster since progress updates are a lot more frequent.
One final concern I had is that I noticed that touch_manifest now
performs a GET instead of a HEAD (I checked the depot server logs).
Was this a limitation of pycurl? The 'HEAD' operations should be
faster for the server...
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
