http: Sleeper, BackOff, and ExponentialBackOff

http: Sleeper, BackOff, and ExponentialBackOff

http issue 206: New BackOff interface and ExponentialBackOff implementation
https://code.google.com/p/google-http-java-client/issues/detail?id=206

http issue 205: New Sleeper interface
https://code.google.com/p/google-http-java-client/issues/detail?id=205

[peleyal, 2013-03-12 20:07:56]

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/util/BackOffUtils.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/BackOffUtils.java:32:
* If {@code true}, it will call {@link Thread#sleep(long)} with the specified
number of
change to: ... "it will call {@link Sleeper#sleep(long)}"

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java:29:
public class FixedBackOff implements BackOff {
I think that we should supply another parameter (similar to maxElapsedTimeMillis
on ExponentialBackOf).

Actually why not adding it to the BackOff interface? - it will be really useful
for testing

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java:55:
return new FixedBackOff(0);
[optional] you can store a variable and return the same instance. same for stop
method

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java:41:
}
If you add maxElapsedTimeMillis to BackOff it will be great to add a test
similar to the following:

while (BackOffUtils.next(sleeper, new FixedBackOff(millis)))
assertEquals(some_number, sleeper.getCount())

[peleyal, 2013-03-12 20:25:56]

another small comment

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/util/FixedBackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/util/FixedBackOffTest.java:26:
public void testGetNextBackOffMillis_zero() {
rename the method (should not contain _zero in it)

[yanivi, 2013-03-13 15:54:54]

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/util/BackOffUtils.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/BackOffUtils.java:32:
* If {@code true}, it will call {@link Thread#sleep(long)} with the specified
number of
On 2013/03/12 20:07:56, peleyal wrote:
> change to: ... "it will call {@link Sleeper#sleep(long)}"

Done.

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java:29:
public class FixedBackOff implements BackOff {
On 2013/03/12 20:07:56, peleyal wrote:
> I think that we should supply another parameter (similar to
maxElapsedTimeMillis
> on ExponentialBackOf).
> 
> Actually why not adding it to the BackOff interface? - it will be really
useful
> for testing

I decided to just eliminate FixedBackOff and replace it with MockBackOff. You
can always just use ExponentialBackOff with multiplier = 1.

I don't understand what you are proposing regarding maxElapsedTimeMillis.  Can
you please explain.

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/FixedBackOff.java:55:
return new FixedBackOff(0);
On 2013/03/12 20:07:56, peleyal wrote:
> [optional] you can store a variable and return the same instance. same for
stop
> method

Done.

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java:41:
}
On 2013/03/12 20:07:56, peleyal wrote:
> If you add maxElapsedTimeMillis to BackOff it will be great to add a test
> similar to the following:
> 
> while (BackOffUtils.next(sleeper, new FixedBackOff(millis)))
> assertEquals(some_number, sleeper.getCount())

Done.

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/util/FixedBackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/9001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/util/FixedBackOffTest.java:26:
public void testGetNextBackOffMillis_zero() {
On 2013/03/12 20:25:56, peleyal wrote:
> rename the method (should not contain _zero in it)

Unfortunately, that's the Google test method name policy.  To be honest, I don't
agree with it, but more important to me is the idea of following a single style
standard.

[peleyal, 2013-03-13 16:59:41]

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/testing/util/MockBackOff.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/testing/util/MockBackOff.java:89:
public final int getNumberOfRetries() {
[optional] as discussed, I prefer getNumberOfTries().

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/testing/util/MockBackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/testing/util/MockBackOffTest.java:30:
public void testGetNextBackOffMillis_zero() throws IOException {
1. I thought that you add _zero only for a special case. Not all test method
include _[something]

2. the method you are testing is nextBackOffMillis, so you should rename it to
testNextBackOfMillis.

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/BackOffTest.java:28:
public void testGetNextBackOffMillis_zero() throws IOException {
ditto

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java:42:
while (retries < backOff.getMaxRetries() + 2) {
[optional]
I prefer 
while (retries <= backOff.getMaxRetires + 1) {

OR

add a comment explaining that the first time doesn't count as retry, or
something else to make it clearer.

[yanivi, 2013-03-13 18:49:28]

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/testing/util/MockBackOff.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/testing/util/MockBackOff.java:89:
public final int getNumberOfRetries() {
On 2013/03/13 16:59:41, peleyal wrote:
> [optional] as discussed, I prefer getNumberOfTries().

Done.

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/testing/util/MockBackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/testing/util/MockBackOffTest.java:30:
public void testGetNextBackOffMillis_zero() throws IOException {
On 2013/03/13 16:59:41, peleyal wrote:
> 1. I thought that you add _zero only for a special case. Not all test method
> include _[something]
> 
> 2. the method you are testing is nextBackOffMillis, so you should rename it to
> testNextBackOfMillis.

Done.

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/BackOffTest.java:28:
public void testGetNextBackOffMillis_zero() throws IOException {
On 2013/03/13 16:59:41, peleyal wrote:
> ditto

Done.

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java
(right):

https://codereview.appspot.com/7443045/diff/24001/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/BackOffUtilsTest.java:42:
while (retries < backOff.getMaxRetries() + 2) {
On 2013/03/13 16:59:41, peleyal wrote:
> [optional]
> I prefer 
> while (retries <= backOff.getMaxRetires + 1) {
> 
> OR
> 
> add a comment explaining that the first time doesn't count as retry, or
> something else to make it clearer.

Done.

[peleyal, 2013-03-13 19:12:45]

LGTM