Add Exponential Backoff to Batch

To fix http://code.google.com/p/google-api-java-client/issues/detail?id=512 : Batch with exponential backoff.

[yanivi, 2012-06-15 15:54:21]

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:81:
public final class BatchRequest {
document that redirects are not followed, and file a feature request to
implement redirects in the future

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:184:
int numRetries = -1;
more elegant would be to use a retriesRemaining variable, so you can simply do
retriesRemaining > 0.  that's what we do in HttpRequest, and yes I am
intentionally trying to make the two code bases look as similar as possible,
just because it makes it easier to maintain.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:189:
HttpRequest batchRequest = buildHttpRequest();
I think you should simplify your logic and only call buildHttpRequest at
initialization time outside the do-while loop.  The only thing that changes
between retries is the content, so you can call setContent here instead.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:195:
(0 < numRetries && numRetries <= batchRequest.getNumberOfRetries()) ? true :
false;
remove the "? true : false"

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:64:
/** Indicates if back off is required before retries. */
"before the next retry"

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:71:
private boolean retryAllowed;
final

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:177:
} else if (retryAllowed && unsuccessfulResponseHandler != null
The logic here is wrong in several ways.  Please take a very careful look at the
implementation in HttpRequest.execute().  I think it should look roughly like
this:

      boolean retrySupported = retryAllowed && (content == null ||
content.retrySupported());
      boolean errorHandled = false;
      if (unsuccessfulResponseHandler != null) {
        errorHandled = unsuccessfulResponseHandler.handleResponse(
            requestInfo.request, response, retrySupported);
      }
      if (!errorHandled && retrySupported && backOffPolicy != null
          && backOffPolicy.isBackOffRequired(response.getStatusCode())) {
        backOffRequired = true;
      }
      if (retrySupported && (errorHandled || backOffRequired)) {
        unsuccessfulRequestInfos.add(requestInfo);
      }

[rmistry, 2012-06-15 17:24:35]

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:81:
public final class BatchRequest {
On 2012/06/15 15:54:21, yanivi wrote:
> document that redirects are not followed, and file a feature request to
> implement redirects in the future

Done. Filed:
http://code.google.com/p/google-api-java-client/issues/detail?id=525

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:184:
int numRetries = -1;
On 2012/06/15 15:54:21, yanivi wrote:
> more elegant would be to use a retriesRemaining variable, so you can simply do
> retriesRemaining > 0.  that's what we do in HttpRequest, and yes I am
> intentionally trying to make the two code bases look as similar as possible,
> just because it makes it easier to maintain.

Done.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:189:
HttpRequest batchRequest = buildHttpRequest();
On 2012/06/15 15:54:21, yanivi wrote:
> I think you should simplify your logic and only call buildHttpRequest at
> initialization time outside the do-while loop.  The only thing that changes
> between retries is the content, so you can call setContent here instead.

Done.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:195:
(0 < numRetries && numRetries <= batchRequest.getNumberOfRetries()) ? true :
false;
On 2012/06/15 15:54:21, yanivi wrote:
> remove the "? true : false"

Done.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:64:
/** Indicates if back off is required before retries. */
On 2012/06/15 15:54:21, yanivi wrote:
> "before the next retry"

Done.

http://codereview.appspot.com/6305096/diff/1/google-api-client/src/main/java/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:71:
private boolean retryAllowed;
On 2012/06/15 15:54:21, yanivi wrote:
> final

Done.

[yanivi, 2012-06-15 18:59:14]

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:187:
boolean retryAllowed = false;
remove " = false"

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:190:
BackOffPolicy backOffPolicy = batchRequest.getBackOffPolicy();
may need this logic, just in case the backoff policy instance is reused across
requests:

    if (backOffPolicy != null) {
      // Reset the BackOffPolicy at the start of each execute.
      backOffPolicy.reset();
    }

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
HttpResponse executeUnparsed() throws IOException {
please remove executeUnparsed method

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:261:
HttpRequest buildHttpRequest() throws IOException {
just inline this method; it doesn't serve any purpose by itself, and it makes it
harder to follow the logic when it is a separate method

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:263:
HttpContent content = new MultipartMixedContent(requestInfos,
"__END_OF_PART__");
remove this line since this is already done on line 194

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:151:
// No point in parsing if there is no callback.
true, but we still need to check for retries, so this returns too early

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:171:
if (requestInfo.dataClass != Void.class) {
The code for parsing (lines 170-175) is almost entirely duplicated. Please move
it into a reusable method that takes Class<A>, and move lines 161-164 in there
also.

[rmistry, 2012-06-15 19:57:43]

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:187:
boolean retryAllowed = false;
On 2012/06/15 18:59:14, yanivi wrote:
> remove " = false"

Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:190:
BackOffPolicy backOffPolicy = batchRequest.getBackOffPolicy();
On 2012/06/15 18:59:14, yanivi wrote:
> may need this logic, just in case the backoff policy instance is reused across
> requests:
> 
>     if (backOffPolicy != null) {
>       // Reset the BackOffPolicy at the start of each execute.
>       backOffPolicy.reset();
>     }

Good idea. Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
HttpResponse executeUnparsed() throws IOException {
On 2012/06/15 18:59:14, yanivi wrote:
> please remove executeUnparsed method

Ah sorry. I meant to. Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:261:
HttpRequest buildHttpRequest() throws IOException {
On 2012/06/15 18:59:14, yanivi wrote:
> just inline this method; it doesn't serve any purpose by itself, and it makes
it
> harder to follow the logic when it is a separate method

Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:263:
HttpContent content = new MultipartMixedContent(requestInfos,
"__END_OF_PART__");
On 2012/06/15 18:59:14, yanivi wrote:
> remove this line since this is already done on line 194

Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:151:
// No point in parsing if there is no callback.
On 2012/06/15 18:59:14, yanivi wrote:
> true, but we still need to check for retries, so this returns too early

Done.

http://codereview.appspot.com/6305096/diff/1004/google-api-client/src/main/ja...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:171:
if (requestInfo.dataClass != Void.class) {
On 2012/06/15 18:59:14, yanivi wrote:
> The code for parsing (lines 170-175) is almost entirely duplicated. Please
move
> it into a reusable method that takes Class<A>, and move lines 161-164 in there
> also.

Done.

[yanivi, 2012-06-15 20:14:26]

LGTM