Http Issue 210: support new back-off handlers (api)

Http Issue 210: support new back-off handlers

Based on https://codereview.appspot.com/7925043/

[peleyal, 2013-03-21 16:32:55]

Hi Yaniv,

BatchRequest is still an open issue, but I prefer to publish this CL now, so we
can talk about it, and you can review MediaUpload and GoogleAccountCredentials.
see my comment in BatchUnparsedResponse.java.

Let me know your thoughts.

[yanivi, 2013-03-28 15:07:46]

some very interesting design issues...

also, please fix all deprecation warnings in Eclipse

https://codereview.appspot.com/7547051/diff/19001/google-api-client-android/s...
File
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:239:
interface GetTokenIOExceptionHandler {
Design issue: this seems to me more complex than it needs to be.  Why not just
use BackOff directly in this class instead of GetTokenIOExceptionHandler?  If
the developer doesn't like it, they are free to override getToken(), but most of
the time BackOff is what they'll want.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:153:
// TODO(peleyal): should be remove, back-off should be handled using
Design issue: this is an excellent point.  It is a serious design flaw in the
way we do batch.  We need to resolve this before we move forward.

I'm thinking that the correct design is actually to eliminate HttpRequest as the
thing that represents the parts.  What we actually want is: String
requestMethod, String url, HttpHeaders headers, HttpParser parser, and 
HttpContent content.

Can you please tackle this as part of this changeset?

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:71:
public final class MediaHttpUploader {
please also fix MediaHttpDownloader

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:576:
* The call back method that will be invoked by
You shouldn't be specifying private classes in public JavaDoc.  Instead just
simplify to say that this method gets called on server error or I/O exception.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:682:
* encountered. Defaults to {@code true}.
@deprecated in JavaDoc

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:27:
* {@link Experimental} <br/>
remove @Experimental from JavaDoc of private API

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:37:
private final MediaHttpUploader uploader;
[optional] I would just make this whole class an anonymous inner class inside
MediaHttpUploader and you won't need this field

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:40:
private HttpIOExceptionHandler originalIOExceptionHandler;
final

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:40:
private HttpIOExceptionHandler originalIOExceptionHandler;
[optional] alternatively, just one field HttpRequest just to simplify the code
(or better yet, no HttpRequest field, and just make this an anon class

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:43:
private HttpUnsuccessfulResponseHandler originalUnsuccessfulHandler;
final

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:58:
// there is no need to call serverErrorCallback if the IOException wasn't
handled
wasn't -> was

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:63:
// DO NOTHING. handled will be returned
do nothing is bad.  instead log a warning, just like we do when retrying on an
IOEXception.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:74:
// there is no need to call serverErrorCallback if the error wasn't handled
wasn't -> was

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:75:
if (handled) {
merge two if-statements

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java:34:
class MediaUploadExponentialBackOffPolicy extends ExponentialBackOffPolicy {
this is package private, so remove it instead of deprecating it.

[peleyal, 2013-04-04 11:39:49]

Hi,
I'm also going to send you shortly a document explaining the options we have to
support httpbackoffunsuccessfulresponsehandler in a batch request

https://codereview.appspot.com/7547051/diff/19001/google-api-client-android/s...
File
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:239:
interface GetTokenIOExceptionHandler {
I changed the method to be not-final, 
and I added backoff field (not back backoffpolicy) which is used in getToken

GetTokenIOExpcetionHandelr was removed!

On 2013/03/28 15:07:46, yanivi wrote:
> Design issue: this seems to me more complex than it needs to be.  Why not just
> use BackOff directly in this class instead of GetTokenIOExceptionHandler?  If
> the developer doesn't like it, they are free to override getToken(), but most
of
> the time BackOff is what they'll want.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:153:
// TODO(peleyal): should be remove, back-off should be handled using
right now I leave it as it is and I'll send a document which contains all the
different options

On 2013/03/28 15:07:46, yanivi wrote:
> Design issue: this is an excellent point.  It is a serious design flaw in the
> way we do batch.  We need to resolve this before we move forward.
> 
> I'm thinking that the correct design is actually to eliminate HttpRequest as
the
> thing that represents the parts.  What we actually want is: String
> requestMethod, String url, HttpHeaders headers, HttpParser parser, and 
> HttpContent content.
> 
> Can you please tackle this as part of this changeset?

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:71:
public final class MediaHttpUploader {
On 2013/03/28 15:07:46, yanivi wrote:
> please also fix MediaHttpDownloader

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:576:
* The call back method that will be invoked by
On 2013/03/28 15:07:46, yanivi wrote:
> You shouldn't be specifying private classes in public JavaDoc.  Instead just
> simplify to say that this method gets called on server error or I/O exception.

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:682:
* encountered. Defaults to {@code true}.
On 2013/03/28 15:07:46, yanivi wrote:
> @deprecated in JavaDoc

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:27:
* {@link Experimental} <br/>
On 2013/03/28 15:07:46, yanivi wrote:
> remove @Experimental from JavaDoc of private API

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:37:
private final MediaHttpUploader uploader;
I prefer to leave it as a separate class. For me anonymous classes are really
tiny, and that class contain some logic in it.
On 2013/03/28 15:07:46, yanivi wrote:
> [optional] I would just make this whole class an anonymous inner class inside
> MediaHttpUploader and you won't need this field

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:40:
private HttpIOExceptionHandler originalIOExceptionHandler;
On 2013/03/28 15:07:46, yanivi wrote:
> final 

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:40:
private HttpIOExceptionHandler originalIOExceptionHandler;
We can't do that, because we are going to change HttpRequest and set this
instance as HttpIOExpcetionHandler and HttpUnsuccessfulResponseHandler.
On 2013/03/28 15:07:46, yanivi wrote:
> [optional] alternatively, just one field HttpRequest just to simplify the code
> (or better yet, no HttpRequest field, and just make this an anon class

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:43:
private HttpUnsuccessfulResponseHandler originalUnsuccessfulHandler;
On 2013/03/28 15:07:46, yanivi wrote:
> final

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:58:
// there is no need to call serverErrorCallback if the IOException wasn't
handled
NO. 
If the IOException wasn't handled we don't need to call server error callback,
because we won't retry.

On 2013/03/28 15:07:46, yanivi wrote:
> wasn't -> was

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:63:
// DO NOTHING. handled will be returned
On 2013/03/28 15:07:46, yanivi wrote:
> do nothing is bad.  instead log a warning, just like we do when retrying on an
> IOEXception.

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:74:
// there is no need to call serverErrorCallback if the error wasn't handled
ditto
On 2013/03/28 15:07:46, yanivi wrote:
> wasn't -> was

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:75:
if (handled) {
On 2013/03/28 15:07:46, yanivi wrote:
> merge two if-statements

Done.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java:34:
class MediaUploadExponentialBackOffPolicy extends ExponentialBackOffPolicy {
OK. I removed that class, but as a result I needed to set IOExceptionHandler and
UnsuccessfulReponseHandler in MediaHttpUploader with Exponential back-off.
Please review...

On 2013/03/28 15:07:46, yanivi wrote:
> this is package private, so remove it instead of deprecating it.

[yanivi, 2013-04-12 21:02:09]

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java:34:
class MediaUploadExponentialBackOffPolicy extends ExponentialBackOffPolicy {
On 2013/04/04 11:39:50, peleyal wrote:
> OK. I removed that class, but as a result I needed to set IOExceptionHandler
and
> UnsuccessfulReponseHandler in MediaHttpUploader with Exponential back-off.
> Please review...
> 
> On 2013/03/28 15:07:46, yanivi wrote:
> > this is package private, so remove it instead of deprecating it.
> 

OK I was totally wrong and you were right.  Sorry!  Please put this code back in
here.

https://codereview.appspot.com/7547051/diff/50001/findbugs-exclude.xml
File findbugs-exclude.xml (right):

https://codereview.appspot.com/7547051/diff/50001/findbugs-exclude.xml#newcode57
findbugs-exclude.xml:57: <Class
name="com.google.api.client.googleapis.media.MediaHttpUploader"/>
[optional] does com.google.api.client.googleapis.media.MediaHttpUploader* work?

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
File
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:57:
public class GoogleAccountCredential implements HttpRequestInitializer {
add a warning in the JavaDoc that explains that previously there was an
exponential backoff policy, but now it is off by default, and you need to call
setBackOff(ExponentialBackOff) to enable it.

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:198:
* Sets the sleeper.
specify that the default is Sleeper.DEFAULT

similarly for all other setSleeper() methods in the library

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:252:
if (backOff == null || !BackOffUtils.next(sleeper, backOff)) throw e;
java style issue: use braces and put "throw e;" on its own line

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:47:
public final class MediaHttpDownloader {
we need a warning in the JavaDoc that we are removing the default behavior of
exponential backoff policy.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(left):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:353:
currentRequest.setBackOffPolicy(new MediaUploadExponentialBackOffPolicy(this));
put this code back in there but mark MediaUploadExponentialBackOffPolicy as
deprecated.  we don't want to remove or change deprecated code if we can avoid
it.  it will be totally removed in 1.16

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:76:
public final class MediaHttpUploader {
we need a warning in the JavaDoc that we are removing the default behavior of
exponential backoff policy and retry on I/O exception

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:280:
throw GoogleJsonResponseException.from(jsonFactory, response);
revert this line

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:283:
* </p>
move line 283 to above line 276

this will make the Eclipse formatter behave correctly

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:366:
// get original handlers
all of this code needs to go away.  there is no reason to rewrite deprecated
code.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:406:
currentRequest.setIOExceptionHandler(mediaErrorHandler);
[optional] move lines 406-407 into MediaUploadErrorHandler constructor

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:506:
// allow retry on I/O errors
remove lines 506-507

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
Preconditions.checkNotNull(request);
remove this line since we are already checking it below on line 53

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:52:
this.uploader = uploader;
java style: this.uploader = Preconditions.checkNotNull(uploader);

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
[optional] java style: in general I prefer to shorten 1-line comments to a
single line if possible, e.g. "no need to call serverErrorCallback if I/O
exception wasn't handled by original handler"

similarly below.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
actually, I think we should still try to call serverErrorCallback() because
regardless if developer wants to retry now, developer may want to retry *later*.

similarly below

[peleyal, 2013-04-15 19:26:37]

Hi,
Hope that my upgrade warnings are good enough, cause I spent a lot of time on
them.

I also changed the default behavior of backOffPolicyEnabled to false on both
media upload and download, since we are going to move it on 1.16.
I though that as a first step we should change it to the behavior of 1.16
without removing it, and then on 1.16 we will just remove that field (and its
getters/setters of course)

Hope everything is clear, if not - i'm here.

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7547051/diff/19001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadExponentialBackOffPolicy.java:34:
class MediaUploadExponentialBackOffPolicy extends ExponentialBackOffPolicy {
On 2013/04/12 21:02:09, yanivi wrote:
> On 2013/04/04 11:39:50, peleyal wrote:
> > OK. I removed that class, but as a result I needed to set IOExceptionHandler
> and
> > UnsuccessfulReponseHandler in MediaHttpUploader with Exponential back-off.
> > Please review...
> > 
> > On 2013/03/28 15:07:46, yanivi wrote:
> > > this is package private, so remove it instead of deprecating it.
> > 
> 
> OK I was totally wrong and you were right.  Sorry!  Please put this code back
in
> here.

Done.

https://codereview.appspot.com/7547051/diff/50001/findbugs-exclude.xml
File findbugs-exclude.xml (right):

https://codereview.appspot.com/7547051/diff/50001/findbugs-exclude.xml#newcode57
findbugs-exclude.xml:57: <Class
name="com.google.api.client.googleapis.media.MediaHttpUploader"/>
On 2013/04/12 21:02:09, yanivi wrote:
> [optional] does com.google.api.client.googleapis.media.MediaHttpUploader*
work?

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
File
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:57:
public class GoogleAccountCredential implements HttpRequestInitializer {
On 2013/04/12 21:02:09, yanivi wrote:
> add a warning in the JavaDoc that explains that previously there was an
> exponential backoff policy, but now it is off by default, and you need to call
> setBackOff(ExponentialBackOff) to enable it.

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:198:
* Sets the sleeper.
On 2013/04/12 21:02:09, yanivi wrote:
> specify that the default is Sleeper.DEFAULT
> 
> similarly for all other setSleeper() methods in the library

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client-android/s...
google-api-client-android/src/main/java/com/google/api/client/googleapis/extensions/android/gms/auth/GoogleAccountCredential.java:252:
if (backOff == null || !BackOffUtils.next(sleeper, backOff)) throw e;
On 2013/04/12 21:02:09, yanivi wrote:
> java style issue: use braces and put "throw e;" on its own line

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:47:
public final class MediaHttpDownloader {
OK. I added a new upgrade warning, and changed the default behavior, since it is
going to be removed on 1.16.
I think it's better to change the default behavior in this version, and remove
the backOffPolicyEnabled field in 1.16.

On 2013/04/12 21:02:09, yanivi wrote:
> we need a warning in the JavaDoc that we are removing the default behavior of
> exponential backoff policy.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(left):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:353:
currentRequest.setBackOffPolicy(new MediaUploadExponentialBackOffPolicy(this));
On 2013/04/12 21:02:09, yanivi wrote:
> put this code back in there but mark MediaUploadExponentialBackOffPolicy as
> deprecated.  we don't want to remove or change deprecated code if we can avoid
> it.  it will be totally removed in 1.16

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:76:
public final class MediaHttpUploader {
On 2013/04/12 21:02:09, yanivi wrote:
> we need a warning in the JavaDoc that we are removing the default behavior of
> exponential backoff policy and retry on I/O exception

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:280:
throw GoogleJsonResponseException.from(jsonFactory, response);
On 2013/04/12 21:02:09, yanivi wrote:
> revert this line

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:283:
* </p>
I'm not sure I did the right change, please review
On 2013/04/12 21:02:09, yanivi wrote:
> move line 283 to above line 276
> 
> this will make the Eclipse formatter behave correctly

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:366:
// get original handlers
On 2013/04/12 21:02:09, yanivi wrote:
> all of this code needs to go away.  there is no reason to rewrite deprecated
> code.

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:406:
currentRequest.setIOExceptionHandler(mediaErrorHandler);
On 2013/04/12 21:02:09, yanivi wrote:
> [optional] move lines 406-407 into MediaUploadErrorHandler constructor

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:506:
// allow retry on I/O errors
On 2013/04/12 21:02:09, yanivi wrote:
> remove lines 506-507

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
Preconditions.checkNotNull(request);
On 2013/04/12 21:02:09, yanivi wrote:
> remove this line since we are already checking it below on line 53

Done. I left the Preconditions.checkNotNull(request), because i think it's
clearer than the following:

    originalIOExceptionHandler =
Preconditions.checkNotNull(request).getIOExceptionHandler();
    originalUnsuccessfulHandler = request.getUnsuccessfulResponseHandler();

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:52:
this.uploader = uploader;
On 2013/04/12 21:02:09, yanivi wrote:
> java style: this.uploader = Preconditions.checkNotNull(uploader);

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
On 2013/04/12 21:02:09, yanivi wrote:
> [optional] java style: in general I prefer to shorten 1-line comments to a
> single line if possible, e.g. "no need to call serverErrorCallback if I/O
> exception wasn't handled by original handler"
> 
> similarly below.

Done.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
First of all - if there isn't an original handler or it returns false, there
won't be any retry.

Secondly, how the user can retry *later*? there isn't any 'resume' method.

On 2013/04/12 21:02:09, yanivi wrote:
> actually, I think we should still try to call serverErrorCallback() because
> regardless if developer wants to retry now, developer may want to retry
*later*.
> 
> similarly below

[yanivi, 2013-04-16 03:17:27]

almost there...

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
Preconditions.checkNotNull(request);
On 2013/04/15 19:26:37, peleyal wrote:
> On 2013/04/12 21:02:09, yanivi wrote:
> > remove this line since we are already checking it below on line 53
> 
> Done. I left the Preconditions.checkNotNull(request), because i think it's
> clearer than the following:
> 
>     originalIOExceptionHandler =
> Preconditions.checkNotNull(request).getIOExceptionHandler();
>     originalUnsuccessfulHandler = request.getUnsuccessfulResponseHandler();
> 

no, you should just do:

originalIOExceptionHandler = request.getIOExceptionHandler();

all checkNotNull(request) does is throw a NullPointerException, so it is of zero
benefit over just dereferencing request.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
On 2013/04/15 19:26:37, peleyal wrote:
> First of all - if there isn't an original handler or it returns false, there
> won't be any retry.
> 
> Secondly, how the user can retry *later*? there isn't any 'resume' method.
> 
> On 2013/04/12 21:02:09, yanivi wrote:
> > actually, I think we should still try to call serverErrorCallback() because
> > regardless if developer wants to retry now, developer may want to retry
> *later*.
> > 
> > similarly below
> 

I think you can just call the upload method again.  But you need to know how
much has already been uploaded.

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:45:
* abnormal HTTP response. Starting with version 1.15 setting back-off is done
for an abnormal HTTP
specify that in 1.15 there is no back off by default.  To enable backoff, ...

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:67:
* with version 1.15 setting back-off is done for an abnormal HTTP response and
an I/O exception by
same here: explain the 1.15 back-off is disabled.

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
* <p>
Remove this paragraph. This is package-private class so keep the JavaDoc light.

[yanivi, 2013-04-16 03:35:06]

BatchRequestTest has a few deprecations also.

[peleyal, 2013-04-16 13:01:46]

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
Preconditions.checkNotNull(request);
Got it.
On 2013/04/16 03:17:27, yanivi wrote:
> On 2013/04/15 19:26:37, peleyal wrote:
> > On 2013/04/12 21:02:09, yanivi wrote:
> > > remove this line since we are already checking it below on line 53
> > 
> > Done. I left the Preconditions.checkNotNull(request), because i think it's
> > clearer than the following:
> > 
> >     originalIOExceptionHandler =
> > Preconditions.checkNotNull(request).getIOExceptionHandler();
> >     originalUnsuccessfulHandler = request.getUnsuccessfulResponseHandler();
> > 
> 
> no, you should just do:
> 
> originalIOExceptionHandler = request.getIOExceptionHandler();
> 
> all checkNotNull(request) does is throw a NullPointerException, so it is of
zero
> benefit over just dereferencing request.

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
The upload method gets initiationRequestUrl and the first thing it does is
sending the initial request to the server. So it looks like we can't resume an
upload operation (in that scenario)
On 2013/04/16 03:17:27, yanivi wrote:
> On 2013/04/15 19:26:37, peleyal wrote:
> > First of all - if there isn't an original handler or it returns false, there
> > won't be any retry.
> > 
> > Secondly, how the user can retry *later*? there isn't any 'resume' method.
> > 
> > On 2013/04/12 21:02:09, yanivi wrote:
> > > actually, I think we should still try to call serverErrorCallback()
because
> > > regardless if developer wants to retry now, developer may want to retry
> > *later*.
> > > 
> > > similarly below
> > 
> 
> I think you can just call the upload method again.  But you need to know how
> much has already been uploaded.

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:45:
* abnormal HTTP response. Starting with version 1.15 setting back-off is done
for an abnormal HTTP
On 2013/04/16 03:17:28, yanivi wrote:
> specify that in 1.15 there is no back off by default.  To enable backoff, ...

Done.

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:67:
* with version 1.15 setting back-off is done for an abnormal HTTP response and
an I/O exception by
On 2013/04/16 03:17:28, yanivi wrote:
> same here: explain the 1.15 back-off is disabled.

Done.

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
* <p>
I removed it, but i think JavaDoc is used by our developers\contributors as
well, so it will be nice to have that comment for them.

On 2013/04/16 03:17:28, yanivi wrote:
> Remove this paragraph. This is package-private class so keep the JavaDoc
light.

[yanivi, 2013-04-16 15:55:44]

minor comments...

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
On 2013/04/16 13:01:46, peleyal wrote:
> The upload method gets initiationRequestUrl and the first thing it does is
> sending the initial request to the server. So it looks like we can't resume an
> upload operation (in that scenario)
> On 2013/04/16 03:17:27, yanivi wrote:
> > On 2013/04/15 19:26:37, peleyal wrote:
> > > First of all - if there isn't an original handler or it returns false,
there
> > > won't be any retry.
> > > 
> > > Secondly, how the user can retry *later*? there isn't any 'resume' method.
> > > 
> > > On 2013/04/12 21:02:09, yanivi wrote:
> > > > actually, I think we should still try to call serverErrorCallback()
> because
> > > > regardless if developer wants to retry now, developer may want to retry
> > > *later*.
> > > > 
> > > > similarly below
> > > 
> > 
> > I think you can just call the upload method again.  But you need to know how
> > much has already been uploaded.
> 

How about we compromise on a TODO comment to investigate if we need to call
serverErrorCallback() on !handled?

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/66003/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:51:
* <p>
On 2013/04/16 13:01:46, peleyal wrote:
> I removed it, but i think JavaDoc is used by our developers\contributors as
> well, so it will be nice to have that comment for them.

[fyi]
I understand that, but the code is self-documenting.  Only want to put something
in the JavaDoc that is not obvious from reading the code.  Otherwise, it makes
the JavaDoc difficult to maintain over time.

> 
> On 2013/04/16 03:17:28, yanivi wrote:
> > Remove this paragraph. This is package-private class so keep the JavaDoc
> light.
>

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:45:
* abnormal HTTP response. Starting with version 1.15 to enable back-off for an
abnormal HTTP
split up this paragraph into 2: one that says that back-off is off by default
and explains how to enable it, and then a second paragraph that explains the
difference between the 1.14 and 1.15 behaviors.  Keep in mind that in the 1.16
cycle we will remove the upgrade warning paragraph, so the first paragraph has
to be able to exist on its own.

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:66:
* abnormal HTTP response and there was a regular retry (without back-off) on I/O
exception.
same here.

[peleyal, 2013-04-16 18:31:18]

Hopefully that's all :)

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/50001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:61:
// there is no need to call serverErrorCallback if the I/O exception wasn't
handled by the
SGTM
On 2013/04/16 15:55:44, yanivi wrote:
> On 2013/04/16 13:01:46, peleyal wrote:
> > The upload method gets initiationRequestUrl and the first thing it does is
> > sending the initial request to the server. So it looks like we can't resume
an
> > upload operation (in that scenario)
> > On 2013/04/16 03:17:27, yanivi wrote:
> > > On 2013/04/15 19:26:37, peleyal wrote:
> > > > First of all - if there isn't an original handler or it returns false,
> there
> > > > won't be any retry.
> > > > 
> > > > Secondly, how the user can retry *later*? there isn't any 'resume'
method.
> > > > 
> > > > On 2013/04/12 21:02:09, yanivi wrote:
> > > > > actually, I think we should still try to call serverErrorCallback()
> > because
> > > > > regardless if developer wants to retry now, developer may want to
retry
> > > > *later*.
> > > > > 
> > > > > similarly below
> > > > 
> > > 
> > > I think you can just call the upload method again.  But you need to know
how
> > > much has already been uploaded.
> > 
> 
> How about we compromise on a TODO comment to investigate if we need to call
> serverErrorCallback() on !handled?

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java
(right):

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpDownloader.java:45:
* abnormal HTTP response. Starting with version 1.15 to enable back-off for an
abnormal HTTP
On 2013/04/16 15:55:44, yanivi wrote:
> split up this paragraph into 2: one that says that back-off is off by default
> and explains how to enable it, and then a second paragraph that explains the
> difference between the 1.14 and 1.15 behaviors.  Keep in mind that in the 1.16
> cycle we will remove the upgrade warning paragraph, so the first paragraph has
> to be able to exist on its own.

Done.

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java
(right):

https://codereview.appspot.com/7547051/diff/98001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java:66:
* abnormal HTTP response and there was a regular retry (without back-off) on I/O
exception.
On 2013/04/16 15:55:44, yanivi wrote:
> same here.

Done.

[yanivi, 2013-04-17 16:29:43]

LGTM

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:64:
// TODO(peleyal): figure out was is best practice - call serverErrorCallback
only if I/O
was -> what

similarly below

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:65:
// exception was handled, or call it anyway
anyway -> regardless

similarly below

[peleyal, 2013-04-19 17:58:02]

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java
(right):

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:64:
// TODO(peleyal): figure out was is best practice - call serverErrorCallback
only if I/O
On 2013/04/17 16:29:43, yanivi wrote:
> was -> what
> 
> similarly below

Done.

https://codereview.appspot.com/7547051/diff/107001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaUploadErrorHandler.java:65:
// exception was handled, or call it anyway
On 2013/04/17 16:29:43, yanivi wrote:
> anyway -> regardless
> 
> similarly below

Done.