Issue 369: Change default behavior of an HTTP request

Issue 369: Change default behavior of Http request

[peleyal, 2013-08-06 21:42:57]

Hi Nick,

I worked on changing the default behavior of an HTTP request.

Num of tries was changed to 3, BackOffHandler maximum timespan was changed to 5
(meaning that after it will generated ~1, ~2, ~4 numbers, it will stop because
the next number is ~8 which is bigger than 5)

Exponential back-off is enabled by default to 503 status codes and not for any
5xx or exceptions.

Let me know your thoughts.
Thanks,
Eyal

[peleyal, 2013-08-12 13:07:59]

ping Nicolas :)

[ngmiceli, 2013-08-13 19:31:26]

What's the client library's current default behavior for backoff? Is it enabled
or disabled? The standard for the other languages seems to be "disabled".

In addition, if it is enabled, is there a default scheme? That should be
exponential, with a rate of 2^x (1,2,4,8) at a limit of 3 tries?

Just want to confirm that information.

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:541: // So we
should make minimum of (lg(5) + 1 calls and
service.HttpClient.MessageHandler.NumTries tries)
This is a test with consistent, predictable behavior, so I don't see the point
of the formula here. You know which will come first, the 3 calls or the 5
seconds, so please be explicit in the comments. Rather than "it should do X or
Y", say "it will do X".

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:570: /// several
retries (up to 5 seconds for a single request or 3 tries in total).
Same as above. This comment is far less confusing, but there still is no reason
why the reader should have to guess. Which will come first, the 3 tries limit or
the 5 seconds?

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:601: // So we
should make minimum of (lg(5) + 1 calls and
service.HttpClient.MessageHandler.NumTries tries)
Ditto, confusing comment.

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs (right):

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs:126: /// <summary> An
helper test for testing the <c>GetNextBackOff</c> logic. </summary>
A helper, not "an" :)

[ngmiceli, 2013-08-13 19:36:25]

Also, I get a patch failure on BaseClientService against head. Do you need to
merge in changes?

[peleyal, 2013-08-13 21:46:19]

The default behavior is to have 3 tries maximum (when exponential back-off is
enable by default for 503 errors)

Notice that when pulling latest changes a lot of files were changed (so if you
need my help in the review let me know)

Eyal

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:541: // So we
should make minimum of (lg(5) + 1 calls and
service.HttpClient.MessageHandler.NumTries tries)
On 2013/08/13 19:31:26, ngmiceli wrote:
> This is a test with consistent, predictable behavior, so I don't see the point
> of the formula here. You know which will come first, the 3 calls or the 5
> seconds, so please be explicit in the comments. Rather than "it should do X or
> Y", say "it will do X".

Done.

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:570: /// several
retries (up to 5 seconds for a single request or 3 tries in total).
On 2013/08/13 19:31:26, ngmiceli wrote:
> Same as above. This comment is far less confusing, but there still is no
reason
> why the reader should have to guess. Which will come first, the 3 tries limit
or
> the 5 seconds?

Done.

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:601: // So we
should make minimum of (lg(5) + 1 calls and
service.HttpClient.MessageHandler.NumTries tries)
On 2013/08/13 19:31:26, ngmiceli wrote:
> Ditto, confusing comment.

Done.

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs (right):

https://codereview.appspot.com/12566043/diff/2001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs:126: /// <summary> An
helper test for testing the <c>GetNextBackOff</c> logic. </summary>
On 2013/08/13 19:31:26, ngmiceli wrote:
> A helper, not "an" :)

Done.

[ngmiceli, 2013-08-16 20:26:41]

LGTM