arc: Implement retry logic for transient errors

arc: Implement retry logic for transient errors

BUG=723835
TEST=Manually, emulated different errors and confirmed that opt in
     flow worked as expected.

Review-Url: https://codereview.chromium.org/2887093003
Cr-Commit-Position: refs/heads/master@{#472606}
Committed: https://chromium.googlesource.com/chromium/src/+/6f3598e45c88e71ece4efc04e044aa1759c82cf9

[khmel, 2017-05-17 21:23:39]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2017-05-17 21:23:40]

Hi Xiyuan,

PTAL

Thanks!

[Luis Héctor Chávez, 2017-05-17 21:29:23]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2017-05-17 21:29:24]

drive-by

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:28: constexpr int kRetryCnt = 3;
nit: kMaxRetryAttempts

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:33: constexpr base::TimeDelta
kRetryTimeout = base::TimeDelta::FromSeconds(10);
Do you actually need this? It's the same as kRefreshTokenTimeout.

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.h (right):

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.h:68: void OnFetchersError(const
GoogleServiceAuthError& error);
nit: OnFetcherError

[khmel, 2017-05-17 21:37:20]

Thanks Luis for comments

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:28: constexpr int kRetryCnt = 3;
On 2017/05/17 21:29:24, Luis Héctor Chávez wrote:
> nit: kMaxRetryAttempts

Done.

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:33: constexpr base::TimeDelta
kRetryTimeout = base::TimeDelta::FromSeconds(10);
On 2017/05/17 21:29:24, Luis Héctor Chávez wrote:
> Do you actually need this? It's the same as kRefreshTokenTimeout.

Let keep it separately. It is for different things.

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.h (right):

https://codereview.chromium.org/2887093003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.h:68: void OnFetchersError(const
GoogleServiceAuthError& error);
On 2017/05/17 21:29:24, Luis Héctor Chávez wrote:
> nit: OnFetcherError

Done.

[xiyuan, 2017-05-17 22:09:16]

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:79: retry_attempt_count_ = 0;
Do we add this before the StartFetchers() call in 
ArcAuthContext::OnRefreshTokensLoaded as well? Or move it up after
ResetFetchers()?

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:121: ++retry_attempt_count_;
Not needed for this CL but do you want to use net::BackoffEntry::Policy ?

One example:
https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser...

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:148: VLOG(1) << "Auth context
was successfully prepared after retry.";
nit:  VLOG_IF(1, retry_attempt_count_) << ...

[khmel, 2017-05-17 22:43:56]

Thanks for comments,

PTAL updated version

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:79: retry_attempt_count_ = 0;
On 2017/05/17 22:09:16, xiyuan wrote:
> Do we add this before the StartFetchers() call in 
> ArcAuthContext::OnRefreshTokensLoaded as well? Or move it up after
> ResetFetchers()?

Good point. Moved here up.

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:121: ++retry_attempt_count_;
On 2017/05/17 22:09:16, xiyuan wrote:
> Not needed for this CL but do you want to use net::BackoffEntry::Policy ?
> 
> One example:
>
https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser...

Nice example. Thanks

https://codereview.chromium.org/2887093003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:148: VLOG(1) << "Auth context
was successfully prepared after retry.";
On 2017/05/17 22:09:16, xiyuan wrote:
> nit:  VLOG_IF(1, retry_attempt_count_) << ...

Done.

[xiyuan, 2017-05-17 22:49:56]

lgtm

[khmel, 2017-05-17 22:50:36]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-05-17 22:52:50]

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, 2017-05-17 23:56:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495061436962570,
"parent_rev": "1f05e0767786c4b551c6cd3846dce6c183500747", "commit_rev":
"6f3598e45c88e71ece4efc04e044aa1759c82cf9"}

[commit-bot: I haz the power, 2017-05-17 23:56:43]

Description was changed from

==========
arc: Implement retry logic for transient errors

BUG=723835
TEST=Manually, emulated different errors and confirmed that opt in
     flow worked as expected.
==========

to

==========
arc: Implement retry logic for transient errors

BUG=723835
TEST=Manually, emulated different errors and confirmed that opt in
     flow worked as expected.

Review-Url: https://codereview.chromium.org/2887093003
Cr-Commit-Position: refs/heads/master@{#472606}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f3598e45c88e71ece4efc04e044...
==========

[commit-bot: I haz the power, 2017-05-17 23:56:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6f3598e45c88e71ece4efc04e044...