> > Another small review here: > > > > http://cr.opensolaris.org/~johansen/webrev-4400/ > > Seems fine. It's a little weird to me that ECONNREFUSED gets to be called > a TransferTimedOut exception, since it really isn't a timeout. I'd expect > maybe that the various transport exception instances would have a > "retryable" attribute which you could test when catching them, or maybe a > multiple-inheritance scheme, where you had exceptions like > RetryableTransferContentException which inherited from > RetryableTransportException and TransferContentException, and you could > catch RetryableTransportException to test for retryability.
Let me just reiterate that it doesn't make sense to do any of this post-November, since we're switching transport methods. My guess is that we'll probably have to discard most the current transport implementation. If you haven't looked at the current TransportException schema, I'd encourage you to do so. Dan added a bunch of exceptions when he unified the multiple error output. We know how to cope with anything that's a subclass of TransportException. If the name annoys you, I can introduce yet another type of TransportException. We can call it RetryableTransportException, as you suggested. We catch _all_ TransportExceptions in just 3 different places in the code. Once they're caught, they get bundled into an object that aggregates the errors into a TransportFailures exception. This exception is then raised to the top level where we pretty-print the error messages in some human readable format. At a certain point, it starts to seem silly to add endless distinctions between TransportExceptions if all we're going to do is use them to print error messages at the top level handler. > If you think this is highly desirable for November, go ahead as-is. I > myself might experiment a bit and try to rationalize it a bit further, even > though doing so would push it out. I think this will simplify any software maintenance that we end up having to do on this code. I would resist the urge to classify these errors ad infinitum, since we have a blanket TransportException handler. That said, if you want TransferTimedOut separate from Retryable, it's not a difficult change. I'm fine with such a modification. Thanks for looking at this. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
