Don't retry fetching policies if it's a blocking operation,

Don't retry fetching policies if it's a blocking operation,
don't wait if we will bypass the proxy next time.

BUG=639822
Committed: https://crrev.com/d3ff97fdea62ba0b09a06cf4f730f92b151a2c58
Cr-Commit-Position: refs/heads/master@{#414055}

[Marton Hunyady, 2016-08-23 13:33:59]

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

[Andrew T Wilson (Slow), 2016-08-23 13:46:27]

Can we add a test to ensure we don't retry failed policy fetches?

https://codereview.chromium.org/2275553002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/device_management_service.cc (right):

https://codereview.chromium.org/2275553002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/device_management_service.cc:358: bool
DeviceManagementRequestJobImpl::ShouldRetry(
I think a better change would be to have ShouldRetry() return an enum: NO_RETRY,
RETRY_IMMEDIATELY, RETRY_WITH_DELAY. That way you don't need to store a
retry_with_delay_ variable and you don't need a second IsRetryWithDelay() API.

[Marton Hunyady, 2016-08-23 14:08:36]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 14:08:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2016-08-23 14:09:44]

https://codereview.chromium.org/2275553002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/device_management_service.cc (right):

https://codereview.chromium.org/2275553002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/device_management_service.cc:358: bool
DeviceManagementRequestJobImpl::ShouldRetry(
On 2016/08/23 13:46:27, Andrew T Wilson (Slow) wrote:
> I think a better change would be to have ShouldRetry() return an enum:
NO_RETRY,
> RETRY_IMMEDIATELY, RETRY_WITH_DELAY. That way you don't need to store a
> retry_with_delay_ variable and you don't need a second IsRetryWithDelay() API.

Done.

[Andrew T Wilson (Slow), 2016-08-23 14:53:09]

Pretty good, just think we need a bit more clarity + a test.

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

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:179: enum
RetryMethod { NO_RETRY, RETRY_IMMEDIATELY, RETRY_WITH_DELAY };
In general, enums are not crammed on one line like this (makes it hard to
comment the various types). let's break this up like so:

enum RetryMethod {
  // No retry required for this request
  NO_RETRY,

  // Should retry immediately (no delay)
  RETRY_IMMEDIATELY,
...

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:368: type_ !=
DeviceManagementRequestJob::TYPE_POLICY_FETCH) {
Per our conversation, let's retry TYPE_POLICY_FETCH immediately, and update the
comment above about why we do it.

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:554:
retry_method ==
This is hard to read. Maybe break this out into a helper function:

int DeviceManagementRequestJobImpl::GetRetryDelay(RetryMethod method) {
  switch (method) {
    case RETRY_WITH_DELAY:
      return g_retry_delay_ms << (job->retries_count() - 1);
    case RETRY_IMMEDIATELY:
      return 0;
    default:
      NOTREACHED();
}

Then you can just use delay = job->GetRetryDelay(retry_method); This has the
nice side effect of keeping the retry timing/logic down in the JobImple.

[Marton Hunyady, 2016-08-23 15:59:48]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 16:00:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2016-08-23 16:00:39]

I added a test too. Drew, can you check if it's ok this way? (The test is
supposed to time out if it's failing.)

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

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:179: enum
RetryMethod { NO_RETRY, RETRY_IMMEDIATELY, RETRY_WITH_DELAY };
On 2016/08/23 14:53:08, Andrew T Wilson (Slow) wrote:
> In general, enums are not crammed on one line like this (makes it hard to
> comment the various types). let's break this up like so:
> 
> enum RetryMethod {
>   // No retry required for this request
>   NO_RETRY,
> 
>   // Should retry immediately (no delay)
>   RETRY_IMMEDIATELY,
> ...

Done.

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:368: type_ !=
DeviceManagementRequestJob::TYPE_POLICY_FETCH) {
On 2016/08/23 14:53:08, Andrew T Wilson (Slow) wrote:
> Per our conversation, let's retry TYPE_POLICY_FETCH immediately, and update
the
> comment above about why we do it.

Done.

https://codereview.chromium.org/2275553002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/device_management_service.cc:554:
retry_method ==
On 2016/08/23 14:53:08, Andrew T Wilson (Slow) wrote:
> This is hard to read. Maybe break this out into a helper function:
> 
> int DeviceManagementRequestJobImpl::GetRetryDelay(RetryMethod method) {
>   switch (method) {
>     case RETRY_WITH_DELAY:
>       return g_retry_delay_ms << (job->retries_count() - 1);
>     case RETRY_IMMEDIATELY:
>       return 0;
>     default:
>       NOTREACHED();
> }
> 
> Then you can just use delay = job->GetRetryDelay(retry_method); This has the
> nice side effect of keeping the retry timing/logic down in the JobImple.

Done.

[commit-bot: I haz the power, 2016-08-23 16:48:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 16:48:40]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-08-24 12:37:35]

lgtm

[Marton Hunyady, 2016-08-24 12:38:17]

The CQ bit was checked by hunyadym@chromium.org

[commit-bot: I haz the power, 2016-08-24 12:38:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 12:42:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 12:44:52]

Description was changed from

==========
Don't retry fetching policies if it's a blocking operation,
don't wait if we will bypass the proxy next time.

BUG=639822
==========

to

==========
Don't retry fetching policies if it's a blocking operation,
don't wait if we will bypass the proxy next time.

BUG=639822

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

[commit-bot: I haz the power, 2016-08-24 12:44:53]

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