Add delayed retry to DeviceManagementService requests.

Add delayed retry to DeviceManagementService requests.

If a network related error occurs while executing a request,
DeviceManagementService retries to execute it 3 more times, with
exponential backoff.

BUG=602993
Committed: https://crrev.com/efdd41f9d440254e720d7f2dc1a2958eea5c5df3
Cr-Commit-Position: refs/heads/master@{#393562}

[Marton Hunyady, 2016-05-04 13:35:58]

hunyadym@chromium.org changed reviewers:
+ atwilson@chromium.org, cschuet@chromium.org

[Marton Hunyady, 2016-05-04 13:35:59]

atwilson: PTAL
cschuet: PTAL at enterprise_device_attributes_apitest.cc

[Marton Hunyady, 2016-05-04 14:13:44]

On 2016/05/04 13:35:59, hunyadym wrote:
> atwilson: PTAL
> cschuet: PTAL at enterprise_device_attributes_apitest.cc

Tests are failing with PatchSet 4, so using a mock TaskRunner will be necessary
(probably I will have to go back to PatchSet 3 in most of the things). I will
fix it on Monday.

[Marton Hunyady, 2016-05-09 12:24:39]

On 2016/05/04 14:13:44, hunyadym wrote:
> On 2016/05/04 13:35:59, hunyadym wrote:
> > atwilson: PTAL
> > cschuet: PTAL at enterprise_device_attributes_apitest.cc
> 
> Tests are failing with PatchSet 4, so using a mock TaskRunner will be
necessary
> (probably I will have to go back to PatchSet 3 in most of the things). I will
> fix it on Monday.

I fixed the tests.

[Andrew T Wilson (Slow), 2016-05-10 17:58:33]

LGTM with a few nits.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
File components/policy/core/common/cloud/device_management_service.cc (right):

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:463: if
(initialized_)
Add a thread_checker_.CalledOnValidThread() to this routine as well, to help
ensure that the task_runner is bound to the correct thread.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:474:
DCHECK(thread_checker_.CalledOnValidThread());
This is fine as-is, but I do wonder if perhaps we should clear our
weak_ptr_factory_ to kill any queued tasks here as well.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:513: void
DeviceManagementService::SetRetryDelayForTesting(long retryDelayMs) {
nit: retry_delay_ms

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:531: int delay
= g_retry_delay_ms * (int)exp2(job->retries_count() - 1);
direct cast is bad, use static_cast.

Also, just consider g_retry_delay_ms << (job->retries_count() -1); I'm fine with
exp2() but I tend to see bit shifting more often than exp2() in the codebase.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
File components/policy/core/common/cloud/device_management_service.h (right):

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.h:160: //
timeouting.
nit: "timing out" not "timeouting" :)

[Marton Hunyady, 2016-05-11 09:40:08]

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
File components/policy/core/common/cloud/device_management_service.cc (right):

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:463: if
(initialized_)
On 2016/05/10 17:58:32, Andrew T Wilson (Slow) wrote:
> Add a thread_checker_.CalledOnValidThread() to this routine as well, to help
> ensure that the task_runner is bound to the correct thread.

Done.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:474:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/05/10 17:58:33, Andrew T Wilson (Slow) wrote:
> This is fine as-is, but I do wonder if perhaps we should clear our
> weak_ptr_factory_ to kill any queued tasks here as well.

Done, I invalidated the pointers. (However, this method is only called from a
test currently.)

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:513: void
DeviceManagementService::SetRetryDelayForTesting(long retryDelayMs) {
On 2016/05/10 17:58:33, Andrew T Wilson (Slow) wrote:
> nit: retry_delay_ms

Done.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:531: int delay
= g_retry_delay_ms * (int)exp2(job->retries_count() - 1);
On 2016/05/10 17:58:33, Andrew T Wilson (Slow) wrote:
> direct cast is bad, use static_cast.
> 
> Also, just consider g_retry_delay_ms << (job->retries_count() -1); I'm fine
with
> exp2() but I tend to see bit shifting more often than exp2() in the codebase.

Done.

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
File components/policy/core/common/cloud/device_management_service.h (right):

https://codereview.chromium.org/1928013004/diff/80001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.h:160: //
timeouting.
On 2016/05/10 17:58:33, Andrew T Wilson (Slow) wrote:
> nit: "timing out" not "timeouting" :)

Done.

[cschuet (SLOW), 2016-05-13 15:20:22]

lgtm

https://codereview.chromium.org/1928013004/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1928013004/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:123:
// Set retry delay to prevent timeouts
nit "." at the end.

[Marton Hunyady, 2016-05-13 15:27:27]

https://codereview.chromium.org/1928013004/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1928013004/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:123:
// Set retry delay to prevent timeouts
On 2016/05/13 15:20:22, cschuet (SLOW) wrote:
> nit "." at the end.

Done.

[Marton Hunyady, 2016-05-13 15:27:47]

The CQ bit was checked by hunyadym@chromium.org

[Marton Hunyady, 2016-05-13 15:27:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, cschuet@chromium.org
Link to the patchset: https://codereview.chromium.org/1928013004/#ps120001
(title: "Nit: punctuation")

[commit-bot: I haz the power, 2016-05-13 15:28:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1928013004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1928013004/120001

[commit-bot: I haz the power, 2016-05-13 17:19:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-13 17:20:26]

Description was changed from

==========
Add delayed retry to DeviceManagementService requests.

If a network related error occurs while executing a request,
DeviceManagementService retries to execute it 3 more times, with
exponential backoff.

BUG=602993
==========

to

==========
Add delayed retry to DeviceManagementService requests.

If a network related error occurs while executing a request,
DeviceManagementService retries to execute it 3 more times, with
exponential backoff.

BUG=602993

Committed: https://crrev.com/efdd41f9d440254e720d7f2dc1a2958eea5c5df3
Cr-Commit-Position: refs/heads/master@{#393562}
==========

[commit-bot: I haz the power, 2016-05-13 17:20:27]

Patchset 7 (id:??) landed as
https://crrev.com/efdd41f9d440254e720d7f2dc1a2958eea5c5df3
Cr-Commit-Position: refs/heads/master@{#393562}