Continued improvement to exception handling code

I'm committing this code early since we need it in fairly short order and it seems quite low-risk: familiar design pattern, no semantic difference in any behavior, just cleanup. Still, the line count is somewhat higher than I'm comfortable committing blindly, so I'm putting this out for review. Happy to clean up per anyone's comments as a follow-up.

[zhoresh, 2010-02-10 22:17:50]

LGTM

http://codereview.appspot.com/207059/diff/1/14
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
(right):

http://codereview.appspot.com/207059/diff/1/14#newcode134
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java:134:
response.getHttpStatusCode());
If the external system return 500, should we as well?
maybe 504?

[johnfargo, 2010-02-10 22:36:33]

IMO we should return 500 when there's an Exception within the fetcher, but
504 when translating this to Shindig-emitted status codes, such as here.

Could you help clean this up in your already-open BasicHttpFetcher cleanup
CL?

Thanks,
John

On Wed, Feb 10, 2010 at 2:17 PM, <zhoresh@gmail.com> wrote:

> LGTM
>
>
>
> http://codereview.appspot.com/207059/diff/1/14
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
> (right):
>
> http://codereview.appspot.com/207059/diff/1/14#newcode134
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java:134:
> response.getHttpStatusCode());
> If the external system return 500, should we as well?
> maybe 504?
>
>
> http://codereview.appspot.com/207059/show
>

[Matt T. Proud (mtp), 2010-02-11 10:57:23]

http://codereview.appspot.com/207059/diff/1/14
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
(right):

http://codereview.appspot.com/207059/diff/1/14#newcode134
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java:134:
response.getHttpStatusCode());
On 2010/02/10 22:17:50, zhoresh wrote:
> If the external system return 500, should we as well?
> maybe 504?
There is a small subset of 500s---e.g., 50{2,4}---that we should not demur from
using.  I think a gateway- or proxy-related one is fine.

[johnfargo, 2010-02-11 21:29:06]

On Thu, Feb 11, 2010 at 2:57 AM, <mtp@google.com> wrote:

>
> http://codereview.appspot.com/207059/diff/1/14
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
> (right):
>
> http://codereview.appspot.com/207059/diff/1/14#newcode134
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java:134:
> response.getHttpStatusCode());
> On 2010/02/10 22:17:50, zhoresh wrote:
>
>> If the external system return 500, should we as well?
>> maybe 504?
>>
> There is a small subset of 500s---e.g., 50{2,4}---that we should not
> demur from using.  I think a gateway- or proxy-related one is fine.


Agreed. In my latest CL (http://codereview.appspot.com/207060/show) I've
taken this advice to use SC_BAD_GATEWAY in situations where the request
itself was legitimately formed but Shindig nevertheless couldn't correctly
process the request, in an attempt to honor the spirit of HTTP status code
400 (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html): "The client
SHOULD NOT repeat the request without modifications".

Having done this, I wonder if there are circumstances in which
SC_BAD_REQUEST is being used when BAD_GATEWAY should be. If you've got a
moment to peek around I'd be interested in your thoughts.

Cheers,
John


>
> http://codereview.appspot.com/207059/show
>

[Matt T. Proud (mtp), 2010-02-15 10:35:25]

John,

What you've done makes sense and looks good to me.  Thanks for being
especially mindful of the HTTP response code nuances.

- M.

On Thu, Feb 11, 2010 at 10:29 PM, John Hjelmstad <johnfargo@gmail.com> wrote:
> On Thu, Feb 11, 2010 at 2:57 AM, <mtp@google.com> wrote:
>>
>> http://codereview.appspot.com/207059/diff/1/14
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
>> (right):
>>
>> http://codereview.appspot.com/207059/diff/1/14#newcode134
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java:134:
>> response.getHttpStatusCode());
>> On 2010/02/10 22:17:50, zhoresh wrote:
>>>
>>> If the external system return 500, should we as well?
>>> maybe 504?
>>
>> There is a small subset of 500s---e.g., 50{2,4}---that we should not
>> demur from using.  I think a gateway- or proxy-related one is fine.
>
> Agreed. In my latest CL (http://codereview.appspot.com/207060/show) I've
> taken this advice to use SC_BAD_GATEWAY in situations where the request
> itself was legitimately formed but Shindig nevertheless couldn't correctly
> process the request, in an attempt to honor the spirit of HTTP status code
> 400 (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html): "The client
> SHOULD NOT repeat the request without modifications".
>
> Having done this, I wonder if there are circumstances in which
> SC_BAD_REQUEST is being used when BAD_GATEWAY should be. If you've got a
> moment to peek around I'd be interested in your thoughts.
> Cheers,
> John
>>
>>
>> http://codereview.appspot.com/207059/show
>
>