Http Issue 210: add back-off handlers (http)

Issue 210: add back-off handlers

[yanivi, 2013-03-26 19:44:49]

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:34:
/** The handler back-off. */
simplify description: "back-off policy" and "sleeper"

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:40:
public HttpBackOffIOExceptionHandler(Builder builder) {
missing JavaDoc

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:40:
public HttpBackOffIOExceptionHandler(Builder builder) {
missing public constructor that just takes backOff parameter

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:43:
Preconditions.checkNotNull(backOff);
remove the preconditions checks since they should already be done in builder

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:48:
public BackOff getBackOff() {
final

similarly for getSleeper()

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:77:
* Builder for {@link HttpBackOffIOExceptionHandler}.
 * {@link Experimental} <br/>

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:92:
public Builder() {
change constructor to take required backOff parameter

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:114:
this.backOff = backOff;
this.backOff = Preconditions.checkNotNull(backOff);

similarly for sleeper

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:118:
/** Returns the sleeper. The default value is {@link Sleeper#DEFAULT}. */
remove the "default value" part from getSleeper

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:124:
* Sets the sleeper. The default value is {@link Sleeper#DEFAULT}.
Either move the "default" value" into its own paragraph, or just combine into a
single sentence, e.g. "Sets the sleeper ({@link Sleeper#DEFAULT} by default)."

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
most of the same comments here as for HttpBackOffIOExceptionHandler, so I won't
repeat those here

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:93:
return false;
for readability move the return statement inside catch statement

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:183:
* Interface which defines if back-off is required based on a {@link
HttpResponse}.
 * {@link Experimental} <br/>

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:190:
* A method which will be invoked when an abnormal response is received and
determines if
remove "A method which will be "

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:199:
public BackOffRequired ALWAYES_REQUIRED = new BackOffRequired() {
maybe rename to "ALWAYS"?

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:199:
public BackOffRequired ALWAYES_REQUIRED = new BackOffRequired() {
remove "public"

similarly below

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:206:
* Back-off required implementation which its {@link #isRequired(HttpResponse)}
Line wrapping is off here.  Make sure you're using Eclipse's auto-formatting
feature.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:209:
public BackOffRequired SERVER_ERROR_REQUIRED = new BackOffRequired() {
maybe rename to ON_SERVER_ERROR?

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Interface which handles a {@link IOException} in a HTTP request.
remove "interface which"

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Interface which handles a {@link IOException} in a HTTP request.
a -> an (in 2 places on this line)

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:52:
/**
please add a whitepace line between lines 51 & 52

similarly for the other files

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:53:
* Handler that will be invoked when a {@link IOException} is thrown during a
HTTP request.
ditto

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:54:
* There is a simple rule that one must follow: If you modify the request object
or modify its
add <p> at beginning of this line, add a whitespace line above it, and add </p>
at end of paragraph

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:58:
* @param request Request object that can be read from for context or modified
before retry
do not capitalize first letter in param description

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(left):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:83:
* MyHeaders to process the response:
revert line removal

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:163:
/** HTTP IOException handler or {@code null} for none. */
"I/O exception"

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:188:
* @deprecated (scheduled to be removed in the 1.16). Use
remove the @deprecated usage info because it's private

similarly below

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:194:
private BackOffPolicy backOffPolicy;
please also deprecate the BackOffPolicy interface and its usages

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:706:
* Returns the HTTP IOException handler or {@code null} for none.
 * {@link Experimental} <br/>

similarly elsewhere

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1151:
if (ioExceptionHandler == null || !ioExceptionHandler.handle(this,
retryRequest)) {
merge above 2 if-statements into 1 using &&

[yanivi, 2013-03-26 20:18:47]

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:939:
* Execute the HTTP request and returns the HTTP response.
Please add a paragraph that explains that if execute() is called twice, the
state of the request object isn't cleared, so will continue where it left off. 
For example, getNumberOfRetries() will not be reset to its original value, but
rather keep its value from the end of the previous execute().

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:940:
* <p>
add whitespace line above <p>

similarly below

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:942:
* yet, and must be parsed by the calling code.
</p>

similarly below

[peleyal, 2013-03-27 13:26:54]

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:34:
/** The handler back-off. */
On 2013/03/26 19:44:50, yanivi wrote:
> simplify description: "back-off policy" and "sleeper"

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:40:
public HttpBackOffIOExceptionHandler(Builder builder) {
On 2013/03/26 19:44:50, yanivi wrote:
> missing JavaDoc

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:40:
public HttpBackOffIOExceptionHandler(Builder builder) {
On 2013/03/26 19:44:50, yanivi wrote:
> missing public constructor that just takes backOff parameter

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:43:
Preconditions.checkNotNull(backOff);
On 2013/03/26 19:44:50, yanivi wrote:
> remove the preconditions checks since they should already be done in builder

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:48:
public BackOff getBackOff() {
On 2013/03/26 19:44:50, yanivi wrote:
> final
> 
> similarly for getSleeper()

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:77:
* Builder for {@link HttpBackOffIOExceptionHandler}.
On 2013/03/26 19:44:50, yanivi wrote:
>  * {@link Experimental} <br/>

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:92:
public Builder() {
On 2013/03/26 19:44:50, yanivi wrote:
> change constructor to take required backOff parameter

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:114:
this.backOff = backOff;
On 2013/03/26 19:44:50, yanivi wrote:
> this.backOff = Preconditions.checkNotNull(backOff);
> 
> similarly for sleeper

Done. 
Eventually I removed the back-off setter, cause we get the back-off in the
constructor

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:118:
/** Returns the sleeper. The default value is {@link Sleeper#DEFAULT}. */
I removed it. In ExponentialBackOff.java getters mention what is the default
value, so I though we want similar comments in this class.

I think that it is clearer if both getters and setters contain "The default
value..." section. I can change it back. What do you think?

On 2013/03/26 19:44:50, yanivi wrote:
> remove the "default value" part from getSleeper

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:124:
* Sets the sleeper. The default value is {@link Sleeper#DEFAULT}.
Done. New paragraph.

On 2013/03/26 19:44:50, yanivi wrote:
> Either move the "default" value" into its own paragraph, or just combine into
a
> single sentence, e.g. "Sets the sleeper ({@link Sleeper#DEFAULT} by default)."

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
On 2013/03/26 19:44:50, yanivi wrote:
> most of the same comments here as for HttpBackOffIOExceptionHandler, so I
won't
> repeat those here

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:93:
return false;
On 2013/03/26 19:44:50, yanivi wrote:
> for readability move the return statement inside catch statement

Can't - if backOffRequired.isRequired returns false we need to have a return
statement here.
And I don't think we should add another return statement in the catch (if we
already have one in the end of the method)

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:183:
* Interface which defines if back-off is required based on a {@link
HttpResponse}.
On 2013/03/26 19:44:50, yanivi wrote:
>  * {@link Experimental} <br/>

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:190:
* A method which will be invoked when an abnormal response is received and
determines if
On 2013/03/26 19:44:50, yanivi wrote:
> remove "A method which will be "

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:199:
public BackOffRequired ALWAYES_REQUIRED = new BackOffRequired() {
On 2013/03/26 19:44:50, yanivi wrote:
> maybe rename to "ALWAYS"?

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:199:
public BackOffRequired ALWAYES_REQUIRED = new BackOffRequired() {
On 2013/03/26 19:44:50, yanivi wrote:
> remove "public"
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:206:
* Back-off required implementation which its {@link #isRequired(HttpResponse)}
On 2013/03/26 19:44:50, yanivi wrote:
> Line wrapping is off here.  Make sure you're using Eclipse's auto-formatting
> feature.

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:209:
public BackOffRequired SERVER_ERROR_REQUIRED = new BackOffRequired() {
On 2013/03/26 19:44:50, yanivi wrote:
> maybe rename to ON_SERVER_ERROR?

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Interface which handles a {@link IOException} in a HTTP request.
On 2013/03/26 19:44:50, yanivi wrote:
> remove "interface which"

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Interface which handles a {@link IOException} in a HTTP request.
On 2013/03/26 19:44:50, yanivi wrote:
> a -> an (in 2 places on this line)

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:52:
/**
On 2013/03/26 19:44:50, yanivi wrote:
> please add a whitepace line between lines 51 & 52
> 
> similarly for the other files

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:53:
* Handler that will be invoked when a {@link IOException} is thrown during a
HTTP request.
On 2013/03/26 19:44:50, yanivi wrote:
> ditto

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:54:
* There is a simple rule that one must follow: If you modify the request object
or modify its
On 2013/03/26 19:44:50, yanivi wrote:
> add <p> at beginning of this line, add a whitespace line above it, and add
</p>
> at end of paragraph

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:58:
* @param request Request object that can be read from for context or modified
before retry
On 2013/03/26 19:44:50, yanivi wrote:
> do not capitalize first letter in param description

That part was copied from HttpUnsuccessfulReponseHandler
Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(left):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:83:
* MyHeaders to process the response:
OOPS
On 2013/03/26 19:44:50, yanivi wrote:
> revert line removal

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:163:
/** HTTP IOException handler or {@code null} for none. */
On 2013/03/26 19:44:50, yanivi wrote:
> "I/O exception"

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:188:
* @deprecated (scheduled to be removed in the 1.16). Use
On 2013/03/26 19:44:50, yanivi wrote:
> remove the @deprecated usage info because it's private
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:194:
private BackOffPolicy backOffPolicy;
On 2013/03/26 19:44:50, yanivi wrote:
> please also deprecate the BackOffPolicy interface and its usages

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:706:
* Returns the HTTP IOException handler or {@code null} for none.
On 2013/03/26 19:44:50, yanivi wrote:
>  * {@link Experimental} <br/>
> 
> similarly elsewhere

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:939:
* Execute the HTTP request and returns the HTTP response.
Done

getNumberOfRetries isn't a good example, because it will remain the same in the
next call (it isn't affected by the execute method).
I added a new paragraph mentioning the state of unsuccessful handler

On 2013/03/26 20:18:47, yanivi wrote:
> Please add a paragraph that explains that if execute() is called twice, the
> state of the request object isn't cleared, so will continue where it left off.

> For example, getNumberOfRetries() will not be reset to its original value, but
> rather keep its value from the end of the previous execute().

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:940:
* <p>
On 2013/03/26 20:18:47, yanivi wrote:
> add whitespace line above <p>
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:942:
* yet, and must be parsed by the calling code.
On 2013/03/26 20:18:47, yanivi wrote:
> </p>
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/4001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:1151:
if (ioExceptionHandler == null || !ioExceptionHandler.handle(this,
retryRequest)) {
On 2013/03/26 19:44:50, yanivi wrote:
> merge above 2 if-statements into 1 using &&

Done.

[yanivi, 2013-03-28 14:11:58]

only minor comments...

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java:22:
* Strategy interface to control back off between retry attempts.
@Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:24:
* Implementation of {@link BackOffPolicy} that increases the back off period for
each retry attempt
@Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:78:
*             {@link HttpBackOffUnsuccessfulResponseHandler} instead.
with ExponentialBackOff

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:265:
* Builder for {@link ExponentialBackOffPolicy}.
@Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:41:
/** Constructs a new instance from a {@link BackOff}. */
missing @param (all constructors should have @param)

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:46:
/** Constructs a new instance from a {@link Builder}. */
missing @param

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:48:
this.backOff = builder.getBackOff();
consistency: builder.backOff

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:98:
/** Constructs a new Builder from a {@link BackOff}. */
missing @param

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
same comments as in other file

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:196:
BackOffRequired ALWAYS_REQUIRED = new BackOffRequired() {
rename to ALWAYS instead of ALWAYS_REQUIRED?  I ask because the context is
BackOffRequired.ALWAYS_REQUIRED which seems redundant.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:203:
* Back-off required implementation which its {@link #isRequired(HttpResponse)}
returns true if
{@code true}

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Handles an {@link IOException} in a HTTP request.
a HTTP -> an HTTP

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:66:
* @return Whether or not this handler has made a change that will require the
request to be
don't capitalize @return description either

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:946:
* {@link HttpUnsuccessfulResponseHandler} attached to this HTTP request will
remind the same as
remind -> remain

[peleyal, 2013-04-02 08:34:13]

Applying your comments and pulling the latest version

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java:22:
* Strategy interface to control back off between retry attempts.
I think that our standard is not adding Experimental to JavaDoc if it's already
deprecated, right? So I leave it as it is

On 2013/03/28 14:11:58, yanivi wrote:
> @Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:24:
* Implementation of {@link BackOffPolicy} that increases the back off period for
each retry attempt
ditto

On 2013/03/28 14:11:58, yanivi wrote:
> @Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:78:
*             {@link HttpBackOffUnsuccessfulResponseHandler} instead.
On 2013/03/28 14:11:58, yanivi wrote:
> with ExponentialBackOff

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/ExponentialBackOffPolicy.java:265:
* Builder for {@link ExponentialBackOffPolicy}.
ditto
On 2013/03/28 14:11:58, yanivi wrote:
> @Experimental

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:41:
/** Constructs a new instance from a {@link BackOff}. */
On 2013/03/28 14:11:58, yanivi wrote:
> missing @param (all constructors should have @param)

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:46:
/** Constructs a new instance from a {@link Builder}. */
On 2013/03/28 14:11:58, yanivi wrote:
> missing @param 

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:48:
this.backOff = builder.getBackOff();
I hate to use internal fields, but it's important to be consistent :(
Done.

On 2013/03/28 14:11:58, yanivi wrote:
> consistency: builder.backOff

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java:98:
/** Constructs a new Builder from a {@link BackOff}. */
On 2013/03/28 14:11:58, yanivi wrote:
> missing @param

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
On 2013/03/28 14:11:58, yanivi wrote:
> same comments as in other file

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:196:
BackOffRequired ALWAYS_REQUIRED = new BackOffRequired() {
On 2013/03/28 14:11:58, yanivi wrote:
> rename to ALWAYS instead of ALWAYS_REQUIRED?  I ask because the context is
> BackOffRequired.ALWAYS_REQUIRED which seems redundant.

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:203:
* Back-off required implementation which its {@link #isRequired(HttpResponse)}
returns true if
On 2013/03/28 14:11:58, yanivi wrote:
> {@code true}

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:24:
* Handles an {@link IOException} in a HTTP request.
On 2013/03/28 14:11:58, yanivi wrote:
> a HTTP -> an HTTP

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:66:
* @return Whether or not this handler has made a change that will require the
request to be
On 2013/03/28 14:11:58, yanivi wrote:
> don't capitalize @return description either

Done.

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:946:
* {@link HttpUnsuccessfulResponseHandler} attached to this HTTP request will
remind the same as
oops

On 2013/03/28 14:11:58, yanivi wrote:
> remind -> remain

[yanivi, 2013-04-02 22:26:46]

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java
(right):

https://codereview.appspot.com/7925043/diff/32001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/BackOffPolicy.java:22:
* Strategy interface to control back off between retry attempts.
Actually, no it isn't the standard.  We should use @Beta annotation in the
JavaDoc even when it is @Deprecated.  I think otherwise, it would be
inconsistent and developers might be confused about whether it is Beta.

On 2013/04/02 08:34:13, peleyal wrote:
> I think that our standard is not adding Experimental to JavaDoc if it's
already
> deprecated, right? So I leave it as it is
> 
> On 2013/03/28 14:11:58, yanivi wrote:
> > @Experimental
>

[peleyal, 2013-04-03 05:31:52]

Done.
I'm pretty sure that in other places we added {@link Beta} in JavaDoc only in
cases it wasn't deprecated API.

[yanivi, 2013-04-12 20:22:31]

LGTM

[yanivi, 2013-04-12 20:52:04]

https://codereview.appspot.com/7925043/diff/58001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/58001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:69:
boolean handle(HttpRequest request, boolean supportsRetry) throws IOException;
naming question: should we instead call it handleIOException?  that way if
something implements both HttpUnsuccessfulResponseHandler and
HttpIOExceptionHandler it makes it easier to understand which one is being
implemented?  See for example MediaUploadErrorHandler.

[peleyal, 2013-04-15 15:28:00]

Please review again - change 'handle' to 'handleIOException' as you suggested.

https://codereview.appspot.com/7925043/diff/58001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
(right):

https://codereview.appspot.com/7925043/diff/58001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java:69:
boolean handle(HttpRequest request, boolean supportsRetry) throws IOException;
Yeah why not. great idea.
On 2013/04/12 20:52:04, yanivi wrote:
> naming question: should we instead call it handleIOException?  that way if
> something implements both HttpUnsuccessfulResponseHandler and
> HttpIOExceptionHandler it makes it easier to understand which one is being
> implemented?  See for example MediaUploadErrorHandler.

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

LGTM

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

I see lots of Eclipse warnings that need to be resolved.  Put @Deprecated on
ExponentialBackOffPolicyTest.  Also use @Deprecated in HttpRequest in the right
places.

Also, think about what we will do in 1.16.  Some of these tests will have to be
re-written.  It may be worth making a copy of each test that has deprecated
code.  @Deprecated on one copy of the test, and a non-deprecated copy of the
test rewritten the new way.  Some methods only exist to test deprecated code and
don't need a copy.  Use your best judgment.

[peleyal, 2013-04-16 14:30:30]

Added tests to HttpRequestTest and deprecated old usages

[yanivi, 2013-04-16 15:47:45]

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:159:
@Override
Sorry, this is a bug in the Eclipse formatter.  please revert this line.

similarly below

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:233:
Assert.assertEquals(1, backOffPolicy.resetCalls);
we should probably add getNumberOfResets() to MockBackOff

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:543:
req.setIOExceptionHandler(new HttpIOExceptionHandler() {
new HttpBackOffIOExceptionHandler(BackOff.ZERO_BACKOFF)

similarly below

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:810:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
remove final

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:853:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
remove final

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:900:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
remove final

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

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:159:
@Override
On 2013/04/16 15:47:45, yanivi wrote:
> Sorry, this is a bug in the Eclipse formatter.  please revert this line.
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:233:
Assert.assertEquals(1, backOffPolicy.resetCalls);
don't think we should do that in this CL, cause we actually don't call reset
from our back-off handlers at all. (maybe in the future we may call reset from
the constructor)

On 2013/04/16 15:47:45, yanivi wrote:
> we should probably add getNumberOfResets() to MockBackOff

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:543:
req.setIOExceptionHandler(new HttpIOExceptionHandler() {
On 2013/04/16 15:47:45, yanivi wrote:
> new HttpBackOffIOExceptionHandler(BackOff.ZERO_BACKOFF)
> 
> similarly below

Done.

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:810:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
On 2013/04/16 15:47:45, yanivi wrote:
> remove final

Done.

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:853:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
On 2013/04/16 15:47:45, yanivi wrote:
> remove final

Done.

https://codereview.appspot.com/7925043/diff/91001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:900:
final MockHttpUnsuccessfulResponseHandler handler = new
MockHttpUnsuccessfulResponseHandler(false);
On 2013/04/16 15:47:45, yanivi wrote:
> remove final

Done.

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

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:28:
* Thread-safe back-off handler which handles abnormal HTTP request with {@link
BackOff}.
As we discussed, this class is not actually thread safe.  Please remove the
Builder class and instead put the setters directly in here.

Change the JavaDoc to explain that it is not thread safe.  It should also
explain that you MUST create a new instance of
HttpBackOffUnsuccessfulResponseHandler and a new instance of BackOff for each
instance of HttpRequest.  It should also explain that BackOff.reset() is not
called.

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
please remove this comment from HttpUnsuccessfulResponseHandler:

 * <p>
 * Implementations should normally be thread-safe.
 * </p>

[peleyal, 2013-04-18 14:49:01]

Done for both HttpBackOffUnsuccessfulReponseHandler and
HttpBackOffIOExceptionHandler.

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:28:
* Thread-safe back-off handler which handles abnormal HTTP request with {@link
BackOff}.
On 2013/04/17 16:27:23, yanivi wrote:
> As we discussed, this class is not actually thread safe.  Please remove the
> Builder class and instead put the setters directly in here.
> 
> Change the JavaDoc to explain that it is not thread safe.  It should also
> explain that you MUST create a new instance of
> HttpBackOffUnsuccessfulResponseHandler and a new instance of BackOff for each
> instance of HttpRequest.  It should also explain that BackOff.reset() is not
> called.

Done.

https://codereview.appspot.com/7925043/diff/102001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:33:
public class HttpBackOffUnsuccessfulResponseHandler implements
HttpUnsuccessfulResponseHandler {
On 2013/04/17 16:27:23, yanivi wrote:
> please remove this comment from HttpUnsuccessfulResponseHandler:
> 
>  * <p>
>  * Implementations should normally be thread-safe.
>  * </p>

Done.

[yanivi, 2013-04-19 17:59:06]

LGTM

https://codereview.appspot.com/7925043/diff/131001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
(right):

https://codereview.appspot.com/7925043/diff/131001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java:30:
* It is designed to work with only one {@link HttpRequest} at a time. As a
result you MUST create a
show example code of how to do this, e.g.

    request.setUnsuccessfulResponseHandler(new
HttpBackOffUnsuccessfulResponseHandler(new ExponentialBackOff()));

[peleyal, 2013-04-19 18:09:29]

done.