Hi Terry

> [1]
> 
> In client.py you have
> 
>          def responseErrback(failure):
>              result.errback(HTTPRequestError(failure))
> 
> I'm not sure why you do it that way. Why not just pass the failure to
> the errback?
> 
> Something went wrong, and you already have a failure, so pass it along.
> 
> If you put the failure into an HTTPRequestError instance and pass that
> to the errback, the errback will wrap the exception instance in a
> failure, which would give a failure containing a exception containing
> a failure containing an exception.
> 
> By unconditionally wrapping every error into an HTTPRequestError
> you're silently turning things that are not HTTP errors (like Python
> syntax errors or the self.errback AttributeError - i.e., things that
> might be/go wrong in responseCallback) into HTTPRequestErrors.

The reason why I wrap all the errors in a HTTPRequestError is because I wanted to have only one kind of error raised by a txsolr request. This is handy in some cases, for example, we can catch HTTPRequestError in the transaction module and retry the transaction if anything wrong happens. This would't be possible if raise any exception because then I don't know what exception to catch. For instance, I don't know what kind of exceptions can be raised by the twisted agent request, and that's not properly documented.

Maybe I can change the code to look like this:

         def responseErrback(failure):
             result.errback(HTTPRequestError(failure.value))

> [2]
> 
> In response.py, I think this should be rewritten:
> 
>     def connectionLost(self, reason):
>         if not reason.check(ResponseDone):
>             _logger.warning('Unclean response: ' + repr(reason.value))
> 
>         try:
>             response = self.responseClass(self.body)
>         except SolrResponseError, e:
>             _logger.error("Can't decode response body: %r" % self.body)
>             self.deferred.errback(e)
>         else:
>             self.deferred.callback(response)
> 
> to
> 
>     def connectionLost(self, reason):
>         if reason.check(ResponseDone):
>             try:
>                 response = self.responseClass(self.body)
>             except SolrResponseError, e:  # better: use Exception
>                 _logger.error("Can't decode response body: %r (%s)" % (
>                     self.body, e))
>                 self.deferred.errback(e)
>             else:
>                 self.deferred.callback(response)
>         else:
>             _logger.warning('Unclean response: ' + repr(reason.value))
>             self.deferred.errback(reason)
> 
> The code as it stands will try to decode the response when you get
> something other than ResponseDone, and you only want to do that when
> the connection is closed properly.

The code I wrote is correct. I want to decode the response even if I don't get a ResponseDone reason. In fact, with our current Solr setup, we never get a ResponseDone reason because the server is using chunked transfer encoding and a Content-Length header is not sent, thus we never get a ResponseDone reason. That's why I try to decode the body first, and only if the procedure fails, I fire the errback, otherwise I'll fire a callback with the decoded response. 

> Question: Are you *sure* that the only possible exception from calling
> self.responseClass(self.body) is a SolrResponseError?  I don't find
> that code very reassuring - what happens if some other exception
> occurs?  I think it would be better to use Exception to catch other
> possible exceptions at that point.  Anything that goes wrong, you give
> it to the errback.

Yes, I agree, I will check for all the possible exceptions and fire an errback if any of them is raised.

> 
> I also added logging of the exception when the response decoding fails
> (I mean printing the exception so we can see what happened).  Ideally
> that kind of error would be translated to an Internal Server Error if
> Fluidinfo ran into it.

Yes I will add an error to the log too.