On 04/29/10 01:17 PM, [email protected] wrote:
On Wed, Apr 28, 2010 at 11:07:08PM -0500, Shawn Walker wrote:
webrev:
http://cr.opensolaris.org/~swalker/pkg-file/
client.py:
- lines 2455 - 2475: What is this code for? The transport shouldn't
raise any urllib exceptions any more. The CatalogRefreshException
is actually a vestige of the previous transport. We can probably
just remove that exception entirely and raise a TransportFailures
if any catalogs are unsuccessfully refreshed.
The CatalogRefreshException is used to bundle up multiple refresh
failures as a single exception and is raised by image.py not in the
transport module, so I think it should stay. I'll go ahead and remove
the isinstance() checks for urllib2 errors though.
transport/engine.py:
- lines 170, 171 and 268, 269: You've defined piplined_protocols.
This might be clearer if written as:
if conn_time == 0 and proto in piplined_protocols:
Yeah; forgot about that. Will fix.
The stats engine accounts for connection time as a separate value.
Have you verified that these changes still lead to appropriate
repository selection when faced with a large number of mirrors and
origins?
I don't quite follow; before we only called record_connection() if
conn_count > 0 and conn_time > 0 and that behaviour is unchanged for
http(s).
The only difference here really is that for some protocols, conn_time
could be 0 (such as file), so a "connection" is just accessing the material.
But as far as I can tell, the repository selection behaviour is still
intact when multiple mirrors or origins are present although I've only
tested with a few, not "large numbers" (not even sure how I would do
that given there aren't many available). And yes, I've looked at what
happens when you mix 'http' and 'file' as well -- file is generally
almost exclusively used and either gets a slightly higher quality number
of a significantly higher one depending on the operation.
If there's a specific sort of test you're looking for, I can run that if
you tell me what it is.
- line 189, 299, 769: How about "if proto not in piplined_protocols" ?
As above; will change.
transport/transport.py:
- lines 176 and 246: Calling this "all" seems like a misnomer, since
you don't get all caches. Instead, you get just the ones that were
configured without a specific publisher. I think this needs a more
self-explanatory name. How about "__any" or "__uncategorized"?
__any it is.
- lines 311, 316, and 317: I'm a little nervous about comparing these
two error codes, especially since there's potential for overlap
between the HTTP response code space and the errno space.
There's currently no overlap for these specific error codes as far as I
know since they should always be the result of something being manually
raised, but I understand the more general concern.
I wonder if we could add some properties to TransportProtoError that
return True if they're the condition we're looking for. E.g. if
e.not_found would be true if the proto is http and the error code is
404, or the proto is file and the error code is ENOENT.
Another possibility would be to create our own set of error codes,
but that might be more complicated.
I think I prefer the idea of adding the property to the exception
object; that's simple and sweet. 'not_found' and 'invalid_request' seem
likely candidates. Oddly enough these seem to be the only places where
they'd be used...
I also just noticed line 316 is wrong since it's using '==' instead of
'in' :( Python's fungability is sometimes annoying...
- lines 1487-1491: Although you've already explained some of this on
lines 289-293, I'd appreciate it if you would amend this comment to
be explicit about the use of perfer_remote. It's a workaround for
one situation where we actually want to go remote if we can. All of
the rest of the code should be using the quality mechanisms in the
transport to decide this. Preferring remote is a special case for
search where it's actually better to go remote first.
Sure; I'll add the "don't use this unless you're doing search" disclaimer ;)
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
