Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Wed, Jul 01, 2009 at 03:06:11PM -0700, david.co...@sun.com wrote: src/pkgdefs/SUNWpython-pycurl/pkginfo Please make the NAME value more descriptive (see pkginfo(4) for more details) since this ends up as the package summary. How about something like NAME="PycURL - Python interface to libcurl" Ok, thanks for clarifying. I'll change NAME to this. On a side note, I know our tradition has been to name things: SUNWpython-foo ...but I've noticed that many packages going into Nevada or SFW (?) have been instead named after this pattern: SUNWpython2x-foo Where x is 4, 5, 6. Is this a concern for us? Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed, Jul 01, 2009 at 05:07:44PM -0500, Shawn Walker wrote: > johan...@sun.com wrote: > >> src/modules/client/transport/engine.py: >>> line 516: where is data defined? did you mean treq.data? >> >> Yes. That's a leftover from converting from a tuple to the >> TransportRequest object. > > Since the client doesn't use POST (that I know of) anymore, does this > mean you need an explicit POST test and test suite just for transport? Search should still be using POST for sending a search that contains multiple queries to the server. If the test suite isn't testing search in this manner, we should probably file a bug to get it improved. >>> line 552: could be a function >> >> I don't understand this comment. Would you please clarify? Line 552 is >> the definition for the function __teardown_handle(). > > This function doesn't use self, so it could be a @classmethod ... Ok, got it. Changed to @staticmethod, since IIRC @classmethod provides the name of the class as an argument. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed, Jul 01, 2009 at 03:06:11PM -0700, david.co...@sun.com wrote: >>> src/pkgdefs/SUNWpython-pycurl/pkginfo >>> >>> Please make the NAME value more descriptive (see pkginfo(4) for >>> more details) since this ends up as the package summary. > > How about something like > > NAME="PycURL - Python interface to libcurl" Ok, thanks for clarifying. I'll change NAME to this. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: >> src/modules/client/transport/engine.py: line 516: where is data defined? did you mean treq.data? Yes. That's a leftover from converting from a tuple to the TransportRequest object. Since the client doesn't use POST (that I know of) anymore, does this mean you need an explicit POST test and test suite just for transport? line 552: could be a function I don't understand this comment. Would you please clarify? Line 552 is the definition for the function __teardown_handle(). This function doesn't use self, so it could be a @classmethod ... src/modules/client/transport/fileobj.py: line 28: unused import The version of the webrev that I'm looking at doesn't have an import here. Nevermind. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
src/pkgdefs/SUNWpython-pycurl/pkginfo Please make the NAME value more descriptive (see pkginfo(4) for more details) since this ends up as the package summary. How about something like NAME="PycURL - Python interface to libcurl" ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Folks, I've integrated the latest feedback from Shawn and David into a new webrev. If anyone would like to take a look at this before I push, please do so soon. http://cr.opensolaris.org/~johansen/webrev-xport-3/ Thanks again, -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Thanks for looking at this, David. On Wed, Jul 01, 2009 at 09:46:41AM -0700, david.co...@sun.com wrote: >> I haven't had any further comments on this webrev. I just updated it to >> include fixes to get distro-import to properly import SUNWpython-pycurl, >> and to make sure the dependency is reflected in SUNWipkg. If no one has >> anything else to add, I'll consider putting this back tomorrow. > > I only looked at the metadata bits. > > src/pkgdefs/Makefile > > Looks like we keep the packages listed in alphabetical order so > please keep this. Fixed SUBDIRS. I didn't see anything else in alphabetical order here. > src/pkgdefs/SUNWipkg/prototype > > Not sure if it the current prototype is sorted by pathname but > it should be. In any case, it looks like your addition isn't > so please move this up prior to the "variant" entries. I re-sorted the entire file instead. > src/pkgdefs/SUNWpython-pycurl/pkginfo > > Please make the NAME value more descriptive (see pkginfo(4) for > more details) since this ends up as the package summary. Would you be more specific about what you'd like this to say? I took a look at pkginfo(4) and the other packages in pkgdefs/*/pkginfo. They all seem to use the package's name as the value for NAME. The package delivers PycURL. I'm a bit lost about what this is supposed to say instead. > src/pkgdefs/SUNWpython-pycurl/prototype > > Same comment as with SUNWipkg about sorting by pathname. Re-sorted as well. > src/util/distro-import/118/common/SUNWpython-pycurl > > Line 1 - That should be "package SUNWpython-pycurl" :-) Thanks for catching that. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed, Jul 01, 2009 at 09:30:20AM -0500, Shawn Walker wrote: > I also encountered this failure in search today with the new transport > patch applied: > > time python ./client.py search -s "http://pkg.opensolaris.org/contrib/"; > "" > > PACKAGE > Traceback (most recent call last): > File "./client.py", line 2594, in ? > __ret = main_func() > File "./client.py", line 2545, in main_func > return search(mydir, pargs) > File "./client.py", line 1081, in search > return_type, pub) > File "./client.py", line 1000, in process_v_1_search > if pub is not None and "prefix" in pub: > TypeError: iterable argument required Gah. That's because this code used to pass the authority around as a dictionary, but we now have a Publisher object. In order to cope with this case, I changed the API code to return either a Publisher object, or in the case where we don't know about the publisher, a RepositoryURI object. I've changed the code in process_v_1_search to cope with what pops back out. I'll test this again before I send out an updated webrev. Thanks for finding this. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Hi Shawn, Thanks for the follow up comments. I've fixed them as indicated if there's no follow-up. > src/modules/client/api_errors.py: > lines 28, 30, 31: these imports are unused now because of transport > changes > > line 320: shouldn't this be returned instead of 's =' ? Yes, you're correct. > src/modules/client/transport/engine.py: > line 255: tx here clashes with your import named the same thing, maybe > safer to use a different name? Thanks. I'll change this to something else. > line 424: str is a python built-in function, better to use a different > name? Thanks, changed. > line 516: where is data defined? did you mean treq.data? Yes. That's a leftover from converting from a tuple to the TransportRequest object. > line 552: could be a function I don't understand this comment. Would you please clarify? Line 552 is the definition for the function __teardown_handle(). > line 608: ultot, ulcur don't appear to be used anywhere The framework specifies what arguments the callback must accept. In this case I'm not using them, but the caller is supplying them, regardless. > src/modules/client/transport/exception.py: > line 59: __cmp__ should exist as an abstract method that raises > NotImplemented in api_errors.TransportError for consistency with line 65? There's a default __cmp__ that comes with the underlying object. I don't think we need to make this an abstract method. In the worst case, we compare by object identity and no harm is done. > lines 296-297: where is .data defined? I don't see it in the __init__ > for InvalidContentException, TransportException, or > api_errors.TransportError. Should this be hasattr(self, "data") and > self.data? I this should be self.reason, thanks for catching that. > src/modules/client/transport/fileobj.py: > line 28: unused import The version of the webrev that I'm looking at doesn't have an import here. > line 58: could be a staticmethod This is the flush() method, which I kept around so this object has a file-like object interface. > src/modules/client/transport/repo.py: > line 305: isn't __repo_cache redundant on a RepoCache object? What > about __cache instead? :) Changed. > line 307: missing self? Yes, fixed. > src/modules/client/transport/transport.py: > lines 136, 334: dir is a python built-in function, maybe use a > different name? Yes, changed. > line 218: you assign this, but never use it? I may have thought I was going to catch another error here. Removed. > line 664: where is the import for zlib? Rhetorical question? :P I've added one. Thanks for the additional comments. I'm going to work through the other feeback that came in this morning and then I'll push a new webrev. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
I haven't had any further comments on this webrev. I just updated it to include fixes to get distro-import to properly import SUNWpython-pycurl, and to make sure the dependency is reflected in SUNWipkg. If no one has anything else to add, I'll consider putting this back tomorrow. I only looked at the metadata bits. src/pkgdefs/Makefile Looks like we keep the packages listed in alphabetical order so please keep this. src/pkgdefs/SUNWipkg/prototype Not sure if it the current prototype is sorted by pathname but it should be. In any case, it looks like your addition isn't so please move this up prior to the "variant" entries. src/pkgdefs/SUNWpython-pycurl/pkginfo Please make the NAME value more descriptive (see pkginfo(4) for more details) since this ends up as the package summary. src/pkgdefs/SUNWpython-pycurl/prototype Same comment as with SUNWipkg about sorting by pathname. src/util/distro-import/118/common/SUNWpython-pycurl Line 1 - That should be "package SUNWpython-pycurl" :-) ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Thu, Jun 25, 2009 at 06:52:13PM -0700, johan...@sun.com wrote: On Tue, Jun 23, 2009 at 01:34:41PM -0700, 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. Thanks to all of you who have provided comments so far. I've incorporated the feedback I've received. The webrev below contains the changes requested in review, as well as a small feature that Shawn requested. http://cr.opensolaris.org/~johansen/webrev-xport-2/ I haven't had any further comments on this webrev. I just updated it to include fixes to get distro-import to properly import SUNWpython-pycurl, and to make sure the dependency is reflected in SUNWipkg. If no one has anything else to add, I'll consider putting this back tomorrow. I also encountered this failure in search today with the new transport patch applied: time python ./client.py search -s "http://pkg.opensolaris.org/contrib/"; "" PACKAGE Traceback (most recent call last): File "./client.py", line 2594, in ? __ret = main_func() File "./client.py", line 2545, in main_func return search(mydir, pargs) File "./client.py", line 1081, in search return_type, pub) File "./client.py", line 1000, in process_v_1_search if pub is not None and "prefix" in pub: TypeError: iterable argument required pkg: This is an internal error. Please let the developers know about this problem by filing a bug at http://defect.opensolaris.org and including the above traceback and this message. The version of pkg(5) is 'b6dc4f84900d+'. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Thu, Jun 25, 2009 at 06:52:13PM -0700, johan...@sun.com wrote: On Tue, Jun 23, 2009 at 01:34:41PM -0700, 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. Thanks to all of you who have provided comments so far. I've incorporated the feedback I've received. The webrev below contains the changes requested in review, as well as a small feature that Shawn requested. http://cr.opensolaris.org/~johansen/webrev-xport-2/ I haven't had any further comments on this webrev. I just updated it to include fixes to get distro-import to properly import SUNWpython-pycurl, and to make sure the dependency is reflected in SUNWipkg. If no one has anything else to add, I'll consider putting this back tomorrow. src/gui/modules/installupdate.py: line 38: unused import now due to removal of URLError from about line 432 src/modules/catalog.py: lines 689-690: content_size doesn't exist anymore lines 693, 711: size isn't used since content_size is gone src/modules/client/api.py: lines 29, 31: these imports are unused now because of transport changes src/modules/client/api_errors.py: lines 28, 30, 31: these imports are unused now because of transport changes line 320: shouldn't this be returned instead of 's =' ? line 327: missing SearchException.__init__(self) line 403: missing IndexingException.__init__(self) src/modules/client/image.py: line 56: unused import due to transport changes src/modules/client/imageconfig.py: line 129: unused now due to transport changes src/modules/updatelog.py: lines 322, 326: unused now due to transport changes src/modules/client/transport/engine.py: line 255: tx here clashes with your import named the same thing, maybe safer to use a different name? line 424: str is a python built-in function, better to use a different name? line 463: space needed after ',' line 516: where is data defined? did you mean treq.data? line 552: could be a function line 608: ultot, ulcur don't appear to be used anywhere src/modules/client/transport/exception.py: line 59: __cmp__ should exist as an abstract method that raises NotImplemented in api_errors.TransportError for consistency with line 65? lines 296-297: where is .data defined? I don't see it in the __init__ for InvalidContentException, TransportException, or api_errors.TransportError. Should this be hasattr(self, "data") and self.data? src/modules/client/transport/fileobj.py: line 28: unused import line 58: could be a staticmethod line 173: list is a python keyword src/modules/client/transport/repo.py: lines 29, 34: unused imports line 305: isn't __repo_cache redundant on a RepoCache object? What about __cache instead? :) line 307: missing self? src/modules/client/transport/stats.py: line 28: unused import src/modules/client/transport/transport.py: lines 136, 334: dir is a python built-in function, maybe use a different name? line 218: you assign this, but never use it? line 376: s/api_errors/apx/ line 481: s/RepoResponse/DepotResponse/ line 573: could be a staticmethod line 664: where is the import for zlib? line 666: s/Invalid/tx.Invalid/ Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 06:52:13PM -0700, johan...@sun.com wrote: > On Tue, Jun 23, 2009 at 01:34:41PM -0700, 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. > > Thanks to all of you who have provided comments so far. I've > incorporated the feedback I've received. The webrev below contains the > changes requested in review, as well as a small feature that Shawn > requested. > > http://cr.opensolaris.org/~johansen/webrev-xport-2/ I haven't had any further comments on this webrev. I just updated it to include fixes to get distro-import to properly import SUNWpython-pycurl, and to make sure the dependency is reflected in SUNWipkg. If no one has anything else to add, I'll consider putting this back tomorrow. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
jmr wrote: Currently the client API implements locking which is a key assumption in how the code is structured in the GUI (we assume the API tasks can take some time so we kick them off in background threads and allow them to update the GUI asynchronously). The only API call that doesn't lock which we need to is the api.info call, the api.info should also acquire the lock as does all the other, plan, prepare, execute and refresh api calls. I disagree. Fundamentally, a single api object can only have one plan. It can be creating, preparing, executing, etc, it, but only one can be in existence. That was why it made sense to implement the locking inside the top level of the api. There's no fundamental reason that info should lock at all. I understand that due to the transport method, it's necessary, but that's entirely different than the plan issue. The locking needed for info is simply an artifact of the changes that are coming in transport. Should this be handled inside the api versus in the client, perhaps, but be aware of the control you'll be giving up over how contention will be resolved. If we place it inside the api, I would like to see this live down in transport area. Brock If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. With regard to the CLI there should be no impact on acquiring the lock as it is carrying out a single linear task and then exciting, unlike the GUI which can be carrying out a number of tasks simultaneously and all have to be managed, including keeping the GUI responsive and up to date. JR johan...@sun.com wrote: On Thu, Jun 25, 2009 at 10:47:40AM -0500, Nicolas Williams wrote: There are plenty of MT-safe APIs that leave locking to the caller (e.g., "only one thread may be active in a given blahblah handle"). Adding locking to the API's implementation makes it easier on threaded callers, but then non-threaded callers pay for needless locking. Which approach is best is generall context-specific. In this case I think the PM GUI can do the locking that the CLI doesn't need, so IMO: leave locking to the caller. After sleeping on this, I agree with Shawn and Nico -- thanks to both of you for chiming in here. If we preemptively put a lock in the transport, the running background thread is going to block any install/update operations that the user may try to perform. Unless it's handled carefully, it will look to the user like the GUI has just hung, which is hardly the user experience that we're aiming for. I would recommend that the GUI quiesce the background thread prior to performing user-requested network operations. (Also, the transport is really cool, and should be a standalone facility for use by any other apps that might come along and need this.) Thanks. The current incarnation is pretty pkg(5) specific, but with more time and polish it could be turned into a more generic set of libraries. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Fri, Jun 26, 2009 at 12:07:13PM +0100, jmr wrote: If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. I'm surprised you haven't filed a bug for this. I just filed: 9715 The info() operation should use the activity_lock My omission - thanks for filing it. With regard to the CLI there should be no impact on acquiring the lock as it is carrying out a single linear task and then exciting, unlike the GUI which can be carrying out a number of tasks simultaneously and all have to be managed, including keeping the GUI responsive and up to date. Ok, but I don't think you answered my concern from the previous e-mail. If the background thread is taking a long time to download manifests, it's going to hold the activity lock and potentially starve out callers who are trying to perform an install/update operation. How do you plan to cope with this scenario? Well we are only fetching small chunks of descriptions now (just the visible rows), so presumably, this should not block for too long. This is being done in the background as far as the UI is concerned so the user can click on Install and it will proceed. The install task itself will appear to the user to have started, the UI will be updating and be redrawn appropriately, but the user is blocked from doing anything else with the modal install dialog. The impact is possibly a slight delay in the initial progress update, until the api.info lock is released and the Install task can grab it and proceed. JR -j johan...@sun.com wrote: On Thu, Jun 25, 2009 at 10:47:40AM -0500, Nicolas Williams wrote: There are plenty of MT-safe APIs that leave locking to the caller (e.g., "only one thread may be active in a given blahblah handle"). Adding locking to the API's implementation makes it easier on threaded callers, but then non-threaded callers pay for needless locking. Which approach is best is generall context-specific. In this case I think the PM GUI can do the locking that the CLI doesn't need, so IMO: leave locking to the caller. After sleeping on this, I agree with Shawn and Nico -- thanks to both of you for chiming in here. If we preemptively put a lock in the transport, the running background thread is going to block any install/update operations that the user may try to perform. Unless it's handled carefully, it will look to the user like the GUI has just hung, which is hardly the user experience that we're aiming for. I would recommend that the GUI quiesce the background thread prior to performing user-requested network operations. (Also, the transport is really cool, and should be a standalone facility for use by any other apps that might come along and need this.) Thanks. The current incarnation is pretty pkg(5) specific, but with more time and polish it could be turned into a more generic set of libraries. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Fri, Jun 26, 2009 at 12:07:13PM +0100, jmr wrote: If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. I'm surprised you haven't filed a bug for this. I just filed: 9715 The info() operation should use the activity_lock With regard to the CLI there should be no impact on acquiring the lock as it is carrying out a single linear task and then exciting, unlike the GUI which can be carrying out a number of tasks simultaneously and all have to be managed, including keeping the GUI responsive and up to date. Ok, but I don't think you answered my concern from the previous e-mail. If the background thread is taking a long time to download manifests, it's going to hold the activity lock and potentially starve out callers who are trying to perform an install/update operation. How do you plan to cope with this scenario? Indeed, if info() holds an activity lock, then install and update operations won't be able to start. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Fri, Jun 26, 2009 at 12:07:13PM +0100, jmr wrote: > If api.info supported locking as does the rest of the API then the issue > we currently have with descriptions and licenses being fetched when a > user goes and does some other API action, would not arise. There might > be a slight delay in the progress starting but the GUI would still be > responsive. The API tasks are all being kicked off in background threads > and calling back to the GUI to update progress and so on. I'm surprised you haven't filed a bug for this. I just filed: 9715 The info() operation should use the activity_lock > With regard to the CLI there should be no impact on acquiring the lock > as it is carrying out a single linear task and then exciting, unlike the > GUI which can be carrying out a number of tasks simultaneously and all > have to be managed, including keeping the GUI responsive and up to date. Ok, but I don't think you answered my concern from the previous e-mail. If the background thread is taking a long time to download manifests, it's going to hold the activity lock and potentially starve out callers who are trying to perform an install/update operation. How do you plan to cope with this scenario? -j > johan...@sun.com wrote: >> On Thu, Jun 25, 2009 at 10:47:40AM -0500, Nicolas Williams wrote: >> >>> There are plenty of MT-safe APIs that leave locking to the caller (e.g., >>> "only one thread may be active in a given blahblah handle"). Adding >>> locking to the API's implementation makes it easier on threaded >>> callers, but then non-threaded callers pay for needless locking. >>> >>> Which approach is best is generall context-specific. In this case I >>> think the PM GUI can do the locking that the CLI doesn't need, so IMO: >>> leave locking to the caller. >>> >> >> After sleeping on this, I agree with Shawn and Nico -- thanks to both of >> you for chiming in here. >> >> If we preemptively put a lock in the transport, the running background >> thread is going to block any install/update operations that the user may >> try to perform. Unless it's handled carefully, it will look to the user >> like the GUI has just hung, which is hardly the user experience that >> we're aiming for. I would recommend that the GUI quiesce the background >> thread prior to performing user-requested network operations. >> >> >>> (Also, the transport is really cool, and should be a standalone facility >>> for use by any other apps that might come along and need this.) >>> >> >> Thanks. The current incarnation is pretty pkg(5) specific, but with >> more time and polish it could be turned into a more generic set of >> libraries. >> >> -j >> ___ >> pkg-discuss mailing list >> pkg-discuss@opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss >> > > ___ > pkg-discuss mailing list > pkg-discuss@opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On 06/26/09 15:55, Shawn Walker wrote: On Jun 26, 2009, at 6:07 AM, jmr wrote: If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. It seems reasonable to have api.info() do locking since it supports queries of multiple packages at a time. However, what happens to the GUI threads that sit there waiting for an api call that may not return for 15-20 minutes while the install/update is going? The user cannot interact with the GUI while install/update is working so if this happens the user will not notice. Padraig ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Jun 26, 2009, at 6:07 AM, jmr wrote: If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. It seems reasonable to have api.info() do locking since it supports queries of multiple packages at a time. However, what happens to the GUI threads that sit there waiting for an api call that may not return for 15-20 minutes while the install/ update is going? -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Currently the client API implements locking which is a key assumption in how the code is structured in the GUI (we assume the API tasks can take some time so we kick them off in background threads and allow them to update the GUI asynchronously). The only API call that doesn't lock which we need to is the api.info call, the api.info should also acquire the lock as does all the other, plan, prepare, execute and refresh api calls. If api.info supported locking as does the rest of the API then the issue we currently have with descriptions and licenses being fetched when a user goes and does some other API action, would not arise. There might be a slight delay in the progress starting but the GUI would still be responsive. The API tasks are all being kicked off in background threads and calling back to the GUI to update progress and so on. With regard to the CLI there should be no impact on acquiring the lock as it is carrying out a single linear task and then exciting, unlike the GUI which can be carrying out a number of tasks simultaneously and all have to be managed, including keeping the GUI responsive and up to date. JR johan...@sun.com wrote: On Thu, Jun 25, 2009 at 10:47:40AM -0500, Nicolas Williams wrote: There are plenty of MT-safe APIs that leave locking to the caller (e.g., "only one thread may be active in a given blahblah handle"). Adding locking to the API's implementation makes it easier on threaded callers, but then non-threaded callers pay for needless locking. Which approach is best is generall context-specific. In this case I think the PM GUI can do the locking that the CLI doesn't need, so IMO: leave locking to the caller. After sleeping on this, I agree with Shawn and Nico -- thanks to both of you for chiming in here. If we preemptively put a lock in the transport, the running background thread is going to block any install/update operations that the user may try to perform. Unless it's handled carefully, it will look to the user like the GUI has just hung, which is hardly the user experience that we're aiming for. I would recommend that the GUI quiesce the background thread prior to performing user-requested network operations. (Also, the transport is really cool, and should be a standalone facility for use by any other apps that might come along and need this.) Thanks. The current incarnation is pretty pkg(5) specific, but with more time and polish it could be turned into a more generic set of libraries. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Tue, Jun 23, 2009 at 01:34:41PM -0700, 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. Thanks to all of you who have provided comments so far. I've incorporated the feedback I've received. The webrev below contains the changes requested in review, as well as a small feature that Shawn requested. http://cr.opensolaris.org/~johansen/webrev-xport-2/ -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 07:38:30PM -0500, Mike Gerdts wrote: > On Tue, Jun 23, 2009 at 3:34 PM, 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. > > How does this affect https_proxy? Libcurl will still respect https_proxy, if you have this set in your environment. I double-checked the code in libcurl 7.19.2. It calls getenv(3C) "_proxy" to check for proxies on all of the protocols that it knows about. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Tue, Jun 23, 2009 at 3:34 PM, 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.
How does this affect https_proxy?
http://defect.opensolaris.org/bz/show_bug.cgi?id=8432
It seems as though it will still be needed, but there seems to be some
unexpected disagreement in environment variables between curl(1) and
libcurl-tutorial(3).
curl(1) says...
http_proxy [protocol://][:port]
Sets proxy server to use for HTTP.
HTTPS_PROXY [protocol://][:port]
Sets proxy server to use for HTTPS.
FTP_PROXY [protocol://][:port]
Sets proxy server to use for FTP.
ALL_PROXY [protocol://][:port]
Sets proxy server to use if no protocol-specific proxy
is set.
NO_PROXY
list of host names that shouldn't go through any proxy.
If set to a asterisk '*' only, it matches all hosts.
libcurl-tutorial(3) says...
The proxy environment variable contents should be in
the format
"[protocol://][user:passw...@]machine[:port]".Where
the protocol:// part is simply ignored if present (so
http://proxy and bluerk://proxy will do the same) and
the optional port number specifies on which port the
proxy operates on the host. If not specified, the
internal default port number will be used and that is
most likely *not* the one you would like it to be.
There are two special environmentvariables.
'all_proxy' is what sets proxy for any URL in case the
protocol specific variable wasn't set, and 'no_proxy'
defines a list of hosts that should not use a proxy
even though a variable may say so. If 'no_proxy' is a
plain asterisk ("*") it matches all hosts.
--
Mike Gerdts
http://mgerdts.blogspot.com/
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Thu, Jun 25, 2009 at 03:25:49PM -0500, Shawn Walker wrote: Some other comments after poking around with the transport api today: * I noticed a ProgressTracker isn't supported when using get_url() (or rather, fetching single resources instead of multiple). Given the size of some our manifests (2.8M for SUNWjruby), I think tracking the download progress would be helpful :) In fact, updating through packagemanager for a new release will trigger a download of at least 65 megabytes of manifest data! This would require some pretty substantial changes to the ProgressTracker and the API, I think. I know the GUI guys want search to be cancelable too. It would probably be better to have byte-based progress tracking for manifests/search/catalog/whatever be its own separate wad, since we'll need to design something that the GUI guys can use, and that fits into the API. It's certainly a legitimate RFE. That's fine, but it would provide a substantial improvement of the user experience for the cli and gui to be able to show this. Especially at the moment since manifest retrieval is such a black box. * It would be good if stats.dump() included the total bytes transferred (which I ended up hacking in my local workspace so I could compare tools.gzip.on to off) The existing format is pretty cramped in an 80 character terminal. I can play with this a bit, but if it makes the output unreadable, I may just omit it for now. A separate RFE is fine. * Maybe stats.dump should be changed to use misc.bytes_to_str and just label the KB/sec column 'Speed' * It would be nice if the additional stats that the object has as private properties (such as __bytes_xfer) could be exposed I should be able to do both of these. Thanks, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 03:25:49PM -0500, Shawn Walker wrote: > Some other comments after poking around with the transport api today: > > * I noticed a ProgressTracker isn't supported when using get_url() (or > rather, fetching single resources instead of multiple). Given the size > of some our manifests (2.8M for SUNWjruby), I think tracking the > download progress would be helpful :) In fact, updating through > packagemanager for a new release will trigger a download of at least 65 > megabytes of manifest data! This would require some pretty substantial changes to the ProgressTracker and the API, I think. I know the GUI guys want search to be cancelable too. It would probably be better to have byte-based progress tracking for manifests/search/catalog/whatever be its own separate wad, since we'll need to design something that the GUI guys can use, and that fits into the API. It's certainly a legitimate RFE. > * It would be good if stats.dump() included the total bytes transferred > (which I ended up hacking in my local workspace so I could compare > tools.gzip.on to off) The existing format is pretty cramped in an 80 character terminal. I can play with this a bit, but if it makes the output unreadable, I may just omit it for now. > * Maybe stats.dump should be changed to use misc.bytes_to_str and just > label the KB/sec column 'Speed' > > * It would be nice if the additional stats that the object has as > private properties (such as __bytes_xfer) could be exposed I should be able to do both of these. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 02:01:47PM -0700, Dan Price wrote: > On Wed 24 Jun 2009 at 05:40PM, johan...@sun.com wrote: > > > 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? > > Ok-- this is something we should fix in pkgsend_bulk. Can you > at least file a bug against the suite? I filed 9688 pkgsend_bulk may send packages too quickly. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed 24 Jun 2009 at 05:40PM, johan...@sun.com wrote: > 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? I was thinking that dump would print str(self). > 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. Ok, the comment will help :) > > 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? Ok-- this is something we should fix in pkgsend_bulk. Can you at least file a bug against the suite? -dp -- Daniel Price, Solaris Kernel Engineeringhttp://blogs.sun.com/dp ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 03:39:47PM -0500, Shawn Walker wrote: > johan...@sun.com wrote: >> On Tue, Jun 23, 2009 at 11:13:18PM -0500, Shawn Walker wrote: >>> 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... >> >> It shouldn't be doing this. You may have found a bug in PyCurl/libcurl. >> In cases where we perform a "HEAD" I tell the handle to set >> CURLOPT_NOBDOY. According to the libcurl documentation, this should >> make the client perform a HEAD. If it's not doing this, then we have a >> bug. I'll investigate this further. > > I believe this is due to: > > modules/client/transport/engine.py: > line 505: this should be an elif instead of 'if' because, based on my > reading of the libcurl docs [1], HEAD should only use pycurl.NOBODY, and > using pycurl.HTTPGET will switch to a GET. Yes, that's right. That was supposed to be an if, elif, else block. I've been staring at this code for too long, I guess. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: On Tue, Jun 23, 2009 at 11:13:18PM -0500, Shawn Walker wrote: 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... It shouldn't be doing this. You may have found a bug in PyCurl/libcurl. In cases where we perform a "HEAD" I tell the handle to set CURLOPT_NOBDOY. According to the libcurl documentation, this should make the client perform a HEAD. If it's not doing this, then we have a bug. I'll investigate this further. I believe this is due to: modules/client/transport/engine.py: line 505: this should be an elif instead of 'if' because, based on my reading of the libcurl docs [1], HEAD should only use pycurl.NOBODY, and using pycurl.HTTPGET will switch to a GET. Cheers, -- Shawn Walker [1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTNOBODY ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
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/ Some other comments after poking around with the transport api today: * I noticed a ProgressTracker isn't supported when using get_url() (or rather, fetching single resources instead of multiple). Given the size of some our manifests (2.8M for SUNWjruby), I think tracking the download progress would be helpful :) In fact, updating through packagemanager for a new release will trigger a download of at least 65 megabytes of manifest data! * It would be good if stats.dump() included the total bytes transferred (which I ended up hacking in my local workspace so I could compare tools.gzip.on to off) * Maybe stats.dump should be changed to use misc.bytes_to_str and just label the KB/sec column 'Speed' * It would be nice if the additional stats that the object has as private properties (such as __bytes_xfer) could be exposed Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Hi Brock,
Kind of you to be our substitute language nurse. ;)
On Thu, Jun 25, 2009 at 02:31:54AM -0700, Brock Pytlik wrote:
> johan...@sun.com wrote:
>>> src/modules/client/image.py:
>>> line 1228: question, what is the difference between 'raise' and
>>> 'raise e'; i've seen both forms used
>>>
>>
>> I'm not sure it matters -- we'd have to ask Danek. I put this in here
>> while trying to debug where one of my InvalidDepotResponse exceptions
>> had gone. I reasoned that since we had the exception assigned to e in
>> the except block, perhaps we should raise it by explicitly naming e.
>>
>>
> I know I'm not Danek, but I'm pretty sure it does make a difference.
> "raise" causes the original exception to have the same traceback as if
> the except block has never happened. "raise e" raises e again, so the
> traceback is based at the raise e line.
> Simple test case demonstrating the difference:
> try:
>raise RuntimeError("foo")
> except e:
>
> If is filled with "raise", you get that exception came from line
> 2, with "raise e", line 4.
>>
>>> src/modules/server/catalog.py:
>>> lines 377-385: is this override actually needed? I thought
>>> __init__ got inherited
>>>
>>
>> I thought that the parent class's __init__ method wasn't called by
>> default, making it necessary to do this. Perhaps Danek can clarify?
>>
>>
> It's not needed, otherwise the exception classes (and client-query
> classes) which simply do "pass" wouldn't work. If you write an init
> method, it becomes your job to call the parent's __init__ method.
Thanks for clarifying both of these points. I'll switch back to using
raise instead of raise e, and will strike the __init__ method for that
class.
-j
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 10:47:40AM -0500, Nicolas Williams wrote: > There are plenty of MT-safe APIs that leave locking to the caller (e.g., > "only one thread may be active in a given blahblah handle"). Adding > locking to the API's implementation makes it easier on threaded > callers, but then non-threaded callers pay for needless locking. > > Which approach is best is generall context-specific. In this case I > think the PM GUI can do the locking that the CLI doesn't need, so IMO: > leave locking to the caller. After sleeping on this, I agree with Shawn and Nico -- thanks to both of you for chiming in here. If we preemptively put a lock in the transport, the running background thread is going to block any install/update operations that the user may try to perform. Unless it's handled carefully, it will look to the user like the GUI has just hung, which is hardly the user experience that we're aiming for. I would recommend that the GUI quiesce the background thread prior to performing user-requested network operations. > (Also, the transport is really cool, and should be a standalone facility > for use by any other apps that might come along and need this.) Thanks. The current incarnation is pretty pkg(5) specific, but with more time and polish it could be turned into a more generic set of libraries. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Thu, Jun 25, 2009 at 10:38:52AM -0500, Shawn Walker wrote: > On Jun 25, 2009, at 7:20 AM, Michal Pryc wrote: > >One solution would be to add the lock mechanism in the GUI, but I am > >not sure if this would be better solution then adding such mechanism > >in the api itself. There is already lock mechanism in the api for > >other purposes. > > While it makes sense for the transport layer to have a lock, shouldn't > the GUI not bother attempting to perform these operations given that > modal nature of the install/update dialogs and to avoid the > unnecessary overhead of a waiting background thread? There are plenty of MT-safe APIs that leave locking to the caller (e.g., "only one thread may be active in a given blahblah handle"). Adding locking to the API's implementation makes it easier on threaded callers, but then non-threaded callers pay for needless locking. Which approach is best is generall context-specific. In this case I think the PM GUI can do the locking that the CLI doesn't need, so IMO: leave locking to the caller. (Also, the transport is really cool, and should be a standalone facility for use by any other apps that might come along and need this.) Nico -- ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Jun 25, 2009, at 7:20 AM, Michal Pryc wrote: Tom Mueller wrote: johan...@sun.com wrote: The transport uses a single libcurl multi-handle, which means that if multiple threads are going to use the transport, we have to serialize callers of multi_perform() for our handle. All of the client is single threaded, and I haven't seen any documentation from the GUI team outlining a multi-threaded approach to downloading. Do you mean a single one for all images, or one per image? (Sorry, I haven't had a chance to review the code yet). The updatetool GUI currently uses multi-threading for evaluating update plans for multiple user images simultaneously. Could this at least be done at the image level to allow this? I have double checked this and it looks like the background thread for getting package descriptions is causing the problem. What is currently happening, when the user is starting PM, the background thread is fired to get descriptions for the packages. This is going to change, so when the user is scrolling to some set of packages, the descriptions are being gathered for the visible rows only. Also when the user performs search, for the visible rows descriptions are being downloaded. At the same time the packae may be installed. So when the description thread is not yet finished and the user will try to install package this problem will occur. Also when the user clicks on the license tab, sometimes it takes ~3 seconds to download the license. During this time if I will hit install button, this error will occur. One solution would be to add the lock mechanism in the GUI, but I am not sure if this would be better solution then adding such mechanism in the api itself. There is already lock mechanism in the api for other purposes. While it makes sense for the transport layer to have a lock, shouldn't the GUI not bother attempting to perform these operations given that modal nature of the install/update dialogs and to avoid the unnecessary overhead of a waiting background thread? Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
Tom Mueller wrote: johan...@sun.com wrote: The transport uses a single libcurl multi-handle, which means that if multiple threads are going to use the transport, we have to serialize callers of multi_perform() for our handle. All of the client is single threaded, and I haven't seen any documentation from the GUI team outlining a multi-threaded approach to downloading. Do you mean a single one for all images, or one per image? (Sorry, I haven't had a chance to review the code yet). The updatetool GUI currently uses multi-threading for evaluating update plans for multiple user images simultaneously. Could this at least be done at the image level to allow this? I have double checked this and it looks like the background thread for getting package descriptions is causing the problem. What is currently happening, when the user is starting PM, the background thread is fired to get descriptions for the packages. This is going to change, so when the user is scrolling to some set of packages, the descriptions are being gathered for the visible rows only. Also when the user performs search, for the visible rows descriptions are being downloaded. At the same time the packae may be installed. So when the description thread is not yet finished and the user will try to install package this problem will occur. Also when the user clicks on the license tab, sometimes it takes ~3 seconds to download the license. During this time if I will hit install button, this error will occur. One solution would be to add the lock mechanism in the GUI, but I am not sure if this would be better solution then adding such mechanism in the api itself. There is already lock mechanism in the api for other purposes. -- best Michal I'll need you to figure out which threads are calling the transport concurrently. We may have to resort to something stupid in the short term, like a transport lock, but I hope a better solution is available. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote:
src/modules/client/image.py:
line 1228: question, what is the difference between 'raise' and 'raise
e'; i've seen both forms used
I'm not sure it matters -- we'd have to ask Danek. I put this in here
while trying to debug where one of my InvalidDepotResponse exceptions
had gone. I reasoned that since we had the exception assigned to e in
the except block, perhaps we should raise it by explicitly naming e.
I know I'm not Danek, but I'm pretty sure it does make a difference.
"raise" causes the original exception to have the same traceback as if
the except block has never happened. "raise e" raises e again, so the
traceback is based at the raise e line.
Simple test case demonstrating the difference:
try:
raise RuntimeError("foo")
except e:
If is filled with "raise", you get that exception came from line
2, with "raise e", line 4.
src/modules/server/catalog.py:
lines 377-385: is this override actually needed? I thought __init__
got inherited
I thought that the parent class's __init__ method wasn't called by
default, making it necessary to do this. Perhaps Danek can clarify?
It's not needed, otherwise the exception classes (and client-query
classes) which simply do "pass" wouldn't work. If you write an init
method, it becomes your job to call the parent's __init__ method.
Hope that helps,
Brock
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
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, johan...@sun.com 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 pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Tue, Jun 23, 2009 at 11:13:18PM -0500, Shawn Walker wrote: > On Jun 23, 2009, at 3:34 PM, johan...@sun.com 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/ Thanks for responding with feedback so quickly. I've fixed the punctuation and spelling errors. Anything else that requires follow-up comments is in line. > 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? Yes, your concern is valid. I thought we had a --no-refresh option that would allow the user to work around this particular case. The situation that's going to catch people is when the catalog is up-to-date and all files are on disk, we'll fail the captive portal test while trying to send intent information. I think the solution is to have an offline mode, or to first install only from cache. However, I probably won't be able to get support for this into the current wad. If you see a more pragmatic option, I'm open to suggestion, though. > line 608: won't a bogus value in PKG_DUMP_STATS, like "true" cause a > traceback here? Thanks. I'll catch a ValueError here. > src/modules/client/api_errors.py: > lines 298, 309: missing an extra newline Thanks. Is the standard two lines between classes? I wasn't sure, since I thought we were okay with just one newline between function definitions. > src/modules/client/image.py: > line 1228: question, what is the difference between 'raise' and 'raise > e'; i've seen both forms used I'm not sure it matters -- we'd have to ask Danek. I put this in here while trying to debug where one of my InvalidDepotResponse exceptions had gone. I reasoned that since we had the exception assigned to e in the except block, perhaps we should raise it by explicitly naming e. > line 1309: the comment says zero, but the code says '= None' This should be None. I've fixed the comment to match. > src/modules/client/imagestate.py: > Nice! This seems like a better home for intent. Thanks for moving > it. As I recall, you helped me find a suitable place to relocate this code. Much obliged for your assistance. > 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 Yes. I had thought I did this, but apparently I forgot. > line 105: nit: shouldn't it be pathname instead of filename if it is > expected to optionally include the path? Yes, in fact filename must be a path. I'll change this accordingly. > 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. Fixed. You were right. There were a couple more cases at the end of the file. > 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. No, you're right. This needs to be fixed. The ex_to_raise case got caught because we failed to cleanup after an exception was raised, but was caught and supressed elsewhere. This lead to us continuing on with fewer and fewer handles available. In the case of the GUI and the CanceledException, we'll always call transport.reset() but this code should be modified to clean up in case that doesn't happen. > 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. I don't really have a preference here. I'm happy to change it. > line 404: should this function validate that size >= 0? My instinct is to say yes, and that we should set a minimum size as well. However, the only code that currently calls this function gets the size by calling statvfs(2), which tells us the optimal fs block size to use. Perhaps if it's given a nonsensical value for the buffer size, it should choose a default like 4 or 8k. > line 554: if you made this 'if hdl.success: continue' you could de- > indent the block below and the formatting would be nicer I would like to, but continue is only valid in a for or while loop. There aren't any loops in __teardown_handle(). :( > src/modules/client/transport/fileobj.py: > > line 247: you hate it that much? Heh. Meant to
Re: [pkg-discuss] Code Review: New Transport
On Tue 23 Jun 2009 at 01:34PM, 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. Here is my initial batch... -- Meta-comments/questions Top level comment: This looks very good. 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. -- 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... -- 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. Also, I didn't really understand the header processing, as I mentioned to you offline... -- 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() ? -- 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. -- 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. -- modules/server/repository.py: Not sure how NastyRepository:catalog is different from its superclass's implementation. I might have missed it somehow? -- SUNWpython-pycurl/Makefile: 31-32-- I know you copied this from somewhere, but please nuke this comment. -- setup.py: why is PCHASH none? -- t_api_search.py: I take it 'sleep(1)' is to ensure unique timestamps? Maybe we should have pkgsend_bulk() keep some state around so that it can avoid this as needed. -- -dp -- Daniel Price, Solaris Kernel Engineeringhttp://blogs.sun.com/dp ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed, Jun 24, 2009 at 01:19:27PM -0500, Tom Mueller wrote: > johan...@sun.com wrote: >> >> The transport uses a single libcurl multi-handle, which means that if >> multiple threads are going to use the transport, we have to serialize >> callers of multi_perform() for our handle. All of the client is single >> threaded, and I haven't seen any documentation from the GUI team >> outlining a multi-threaded approach to downloading. >> > Do you mean a single one for all images, or one per image? (Sorry, I > haven't had a chance to review the code yet). > > The updatetool GUI currently uses multi-threading for evaluating update > plans for multiple user images simultaneously. > Could this at least be done at the image level to allow this? Sorry if I was unclear. The correspondence between curl multi-handle and image is 1:1. In the case you describe, everything should be okay, as long you're using one thread per user image. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
johan...@sun.com wrote: The transport uses a single libcurl multi-handle, which means that if multiple threads are going to use the transport, we have to serialize callers of multi_perform() for our handle. All of the client is single threaded, and I haven't seen any documentation from the GUI team outlining a multi-threaded approach to downloading. Do you mean a single one for all images, or one per image? (Sorry, I haven't had a chance to review the code yet). The updatetool GUI currently uses multi-threading for evaluating update plans for multiple user images simultaneously. Could this at least be done at the image level to allow this? Thanks. Tom I'll need you to figure out which threads are calling the transport concurrently. We may have to resort to something stupid in the short term, like a transport lock, but I hope a better solution is available. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss begin:vcard fn:Tom Mueller n:Mueller;Tom org:Sun Microsystems, Inc.;Update Center Software adr:;;21915 Hillandale Dr;Elkhorn;NE;68022;USA email;internet:tom.muel...@sun.com title:Senior Staff Engineer tel;work:877-250-4011 tel;fax:877-250-4011 tel;home:402-916-9943 x-mozilla-html:TRUE version:2.1 end:vcard ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
On Wed, Jun 24, 2009 at 02:34:49PM +0200, Michal Pryc wrote: > For SUNWgnome-desklets during download stage after 78.43kB downloaded > I've got the following exception. Please note that exception occurred > and the progress still moved ~1kB. This is happening every time for > every package, so I am unable to perform more tests :( > > File > "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", > line 98, in __call_perform >ret, active_handles = self.__mhandle.perform() > error: cannot invoke perform() - multi_perform() is currently running > > File > "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", > line 98, in __call_perform >ret, active_handles = self.__mhandle.perform() > error: cannot invoke perform() - multi_perform() is currently running > > > The pstack on the packagemanager process during this exception is > showing some thread running: > > http://pastebin.com/f3346021a The transport uses a single libcurl multi-handle, which means that if multiple threads are going to use the transport, we have to serialize callers of multi_perform() for our handle. All of the client is single threaded, and I haven't seen any documentation from the GUI team outlining a multi-threaded approach to downloading. I'll need you to figure out which threads are calling the transport concurrently. We may have to resort to something stupid in the short term, like a transport lock, but I hope a better solution is available. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: New Transport
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. Hi, If someone else would like to test those bits, the package SUNWcurl is required to build the gate with the patch from webrev-xport-1. The progress is moving really nice, but I got a problem installing packages. I was testing on build 111b. For SUNWgnome-desklets during download stage after 78.43kB downloaded I've got the following exception. Please note that exception occurred and the progress still moved ~1kB. This is happening every time for every package, so I am unable to perform more tests :( Exception traceback: Traceback (most recent call last): File "/usr/lib/python2.4/vendor-packages/pkg/gui/installupdate.py", line 406, in __proceed_with_stages_thread_ex self.__proceed_with_stages_thread() File "/usr/lib/python2.4/vendor-packages/pkg/gui/installupdate.py", line 565, in __proceed_with_stages_thread self.api_o.prepare() File "/usr/lib/python2.4/vendor-packages/pkg/client/api.py", line 441, in prepare self.img.imageplan.preexecute() File "/usr/lib/python2.4/vendor-packages/pkg/client/imageplan.py", line 632, in preexecute p.download() File "/usr/lib/python2.4/vendor-packages/pkg/client/pkgplan.py", line 244, in download mfile.wait_files() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/transport.py", line 801, in wait_files self._transport._get_files(self) File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/transport.py", line 393, in _get_files errlist = d.get_files(filelist, download_dir, progtrack) File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/repo.py", line 206, in get_files self._engine.run() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", line 332, in run self.__call_perform() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", line 98, in __call_perform ret, active_handles = self.__mhandle.perform() error: cannot invoke perform() - multi_perform() is currently running The second time (without restarting GUI, when I was trying to install the same package, similar exception, but with different stack occured: Exception traceback: Traceback (most recent call last): File "/usr/lib/python2.4/vendor-packages/pkg/gui/installupdate.py", line 406, in __proceed_with_stages_thread_ex self.__proceed_with_stages_thread() File "/usr/lib/python2.4/vendor-packages/pkg/gui/installupdate.py", line 554, in __proceed_with_stages_thread stuff_todo = self.__plan_stage() File "/usr/lib/python2.4/vendor-packages/pkg/gui/installupdate.py", line 723, in __plan_stage filters = []) File "/usr/lib/python2.4/vendor-packages/pkg/client/api.py", line 176, in plan_install filters=filters, verbose=verbose) File "/usr/lib/python2.4/vendor-packages/pkg/client/image.py", line 2609, in make_install_plan ip.evaluate() File "/usr/lib/python2.4/vendor-packages/pkg/client/imageplan.py", line 433, in evaluate self.evaluate_fmri(f) File "/usr/lib/python2.4/vendor-packages/pkg/client/imageplan.py", line 283, in evaluate_fmri m = self.image.get_manifest(pfmri) File "/usr/lib/python2.4/vendor-packages/pkg/client/image.py", line 778, in get_manifest self.__touch_manifest(fmri) File "/usr/lib/python2.4/vendor-packages/pkg/client/image.py", line 726, in __touch_manifest self.state.get_intent_str(fmri)) File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/transport.py", line 248, in touch_manifest d.touch_manifest(mfst, header) File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/repo.py", line 256, in touch_manifest respdata = resp.read() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/fileobj.py", line 69, in read while self.__fill_buffer(): File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/fileobj.py", line 241, in __fill_buffer engine.run() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", line 332, in run self.__call_perform() File "/usr/lib/python2.4/vendor-packages/pkg/client/transport/engine.py", line 98, in __call_perform ret, active_handles = self.__mhandle.perform() error: cannot invoke perform() - multi_perform() is currently running The pstack on the packagemanager process during this exception is showing some thread running: http://pastebin.com/f3346021a -- best Michal Pryc ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http:
Re: [pkg-discuss] Code Review: New Transport
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.trans
