Serving the original response incase of RewriterException for Accel.

The original algorithm to check if the error is recoverable was never working. This removes the unnecessary check and just return the old content.

[gagan.goku, 2011-03-10 08:53:46]

If possible, please add a test for accel handler when rewriting fails and messes
up the results pointer, to make sure that the original results are still correct
and valid.

http://codereview.appspot.com/4240102/diff/1/java/common/conf/shindig.properties
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/4240102/diff/1/java/common/conf/shindig.propert...
java/common/conf/shindig.properties:159:
shindig.accelerate.useOriginalResponseOnError = false
for consistency, no space before and after =

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:100:
results = originalResults;
you dont need originalResults because in case of exception, results will point
to the same http response as it was pointing before the call.
As discussed offline, you need a deep copy of results before the rewriting call

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:52:
public void
setUseErrorResponse(@Named("shindig.accelerate.sendInternalErrorToUser")
isnt this boolean same as !useOriginalResponseOnError
I think both booleans can be clubbed into 1, like debug mode or something.

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:54:
this.sendInternalErrorToUser = sendErrorToUser;
for consistency, call checkInitialized() as well
(though that call is kind of useless)

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:79:
response = new HttpResponseBuilder().setHttpStatusCode(500).create();
SC_INTERNAL_SERVER_ERROR

[atulvasu, 2011-03-14 12:37:20]

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:100:
results = originalResults;
On 2011/03/10 08:53:46, gagan.goku wrote:
> you dont need originalResults because in case of exception, results will point
> to the same http response as it was pointing before the call.
> As discussed offline, you need a deep copy of results before the rewriting
call

Done.

[gagan.goku, 2011-03-14 13:10:10]

!(lgtm)

http://codereview.appspot.com/4240102/diff/5001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/5001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:82:
HttpResponse originalResults = new HttpResponseBuilder(results).create();
again, wont work because there are no changes to the response builder, so
create() will return the original result as an optimization (you cant blame
them, its actually a very good optimization)
Please add a test which messes up the results while rewriting, that should help
a lot.

[atulvasu, 2011-03-15 02:50:13]

http://codereview.appspot.com/4240102/diff/5001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/5001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:82:
HttpResponse originalResults = new HttpResponseBuilder(results).create();
On 2011/03/14 13:10:10, gagan.goku wrote:
> again, wont work because there are no changes to the response builder, so
> create() will return the original result as an optimization (you cant blame
> them, its actually a very good optimization)
> Please add a test which messes up the results while rewriting, that should
help
> a lot.

I over-read too much into your comment, reverting back to originalResults =
results. It works because HttpResponse is immutable.

[gagan.goku, 2011-03-15 03:43:24]

Please add a test Atul, it will be much easier for you really.

http://codereview.appspot.com/4240102/diff/6002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/6002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:91:
results = originalResults;
as discussed on chat, this is not going to help either.
in case of an exception, results pointer is not going to change.

[atulvasu, 2011-03-17 06:17:14]

http://codereview.appspot.com/4240102/diff/6002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/4240102/diff/6002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:91:
results = originalResults;
On 2011/03/15 03:43:24, gagan.goku wrote:
> as discussed on chat, this is not going to help either.
> in case of an exception, results pointer is not going to change.

That is the nice part :) removing this assignment too.

[nikhilmadan23, 2011-03-17 12:58:49]

lgtm

http://codereview.appspot.com/4240102/diff/12002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java
(right):

http://codereview.appspot.com/4240102/diff/12002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:422:
((FakeCaptureRewriter) rewriter).setContentToRewrite(REWRITE_CONTENT);
why do we need this?

[gagan.goku, 2011-03-18 13:40:40]

Lgmt, committing

[gagan.goku, 2011-03-18 15:04:17]

Build looks good.
Committed as r1082941.