On Wed 08 Oct 2008 at 10:39AM, [EMAIL PROTECTED] wrote:
> Hey Dan,
>
> Thanks for making these changes. I like the format of the output. I
> also appreciate you taking the time to make the transport exceptions
> into an orderly hierarchy.
>
> > http://cr.opensolaris.org/~dp/pkg-timeout
>
> Comments below:
>
> client.py:
>
> - lines 1814 and 1819: Since we have a global shared state object,
> perhaps it would make sense to read these enviornment variables when
> we instantiate the global state object instead. This would have the
> benefit of making the environment overrides available to the GUI
> without requiring any additional plumbing on their part.
I agree-- but as mentioned earlier I'm going to defer. Will file a bug.
> - lines 1954 - 1968: I think the ordering of these exception handlers
> is backwards. Since InvalidContentException is a subclass of
> TransportException, the first TransportException handler is going to
> catch _all_ of the TransportExceptions, including InvalidContent. I
> don't think that's what you want. At a minimum, I'd put
> InvalidContent before Transport. However, since the printout for
> the TransportException handler specifically mentions timeouts,
> perhaps what you really wanted to catch here was a
> TransferTimedOutException? I'm not sure.
Great catch. Thanks. Do you think that it's ever possible in
the current code for an InvalidContentException to wind up at
the top level? As best I can tell, InvalidContent can never
propagate to the top level in the current code (as I've altered it).
It's caught in all cases and either silently fixed, or retried. Do you
agree?
> filelist.py:
>
> - line 223: Not directly related to filelist, but a parenthetical
> thought that occured while looking at this code: would it make
> sense to either write accessor methods or attribute methods for the
> global object so we could instead do something like:
>
> retry_count = global_settings.get_max_retry()
>
> or
>
> retry_count = global_settings["PKG_TIMEOUT_MAX"]
>
> I'm not sure either of these are any better than what we have now,
> but I thought I'd toss that out anyway.
Maybe later with the env variable processing rewhack which was
suggested.
> retrieve.py:
>
> - line 251: Out of curiosity, what part of the code is generating a
> ReadError here? I was under the impression that we were likely to
> get an IOError or socket error here. Is this leftover from the
> ValueException fix that you handed off to me?
ReadError is really tarfile.ReadError. So, this is wrong. I will
change it to EnvironmentError.
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
