[Sync] Sync should run canary cycle after any network related error

[Sync] Sync should run canary cycle after any network related error

Today sync only runs canary cycle after small subset of network errors
that are related to connection/network failure. This leaves sync in
exponential backoff when unexpected error is returned from network
stack.

In this change I make sync translate all network errors to
CONNECTION_UNAVAILABLE therefore making it try canary job after network
change.

BUG=539943
R=zea@chromium.org

Committed: https://crrev.com/bcaf62bdd2b84e26cf419ea483584cc6ef918330
Cr-Commit-Position: refs/heads/master@{#367746}

[pavely, 2015-12-29 18:52:45]

I've adjusted initial delay for NETWORK_UNAVAILABLE errors in
BackoffDelayProvvider. With initial delay reduced to 30 seconds and scheduler
running canary job on network change I don't think we need to retry on network
errors immediately like it is implemented right now.

[Nicolas Zea, 2015-12-30 21:34:35]

Seems like this would be a good case for an integration test. Do you know if we
have any easy way of testing the backoff + error response behavior?

https://codereview.chromium.org/1553433002/diff/20001/sync/internal_api/synca...
File sync/internal_api/syncapi_server_connection_manager.cc (right):

https://codereview.chromium.org/1553433002/diff/20001/sync/internal_api/synca...
sync/internal_api/syncapi_server_connection_manager.cc:56: DCHECK(error_code !=
net::OK);
DCHECK_NE?

[pavely, 2016-01-04 23:31:04]

I've updated SyncExponentialBackoffTest to check that canary job is triggered
after network change.

https://codereview.chromium.org/1553433002/diff/20001/sync/internal_api/synca...
File sync/internal_api/syncapi_server_connection_manager.cc (right):

https://codereview.chromium.org/1553433002/diff/20001/sync/internal_api/synca...
sync/internal_api/syncapi_server_connection_manager.cc:56: DCHECK(error_code !=
net::OK);
On 2015/12/30 21:34:34, Nicolas Zea wrote:
> DCHECK_NE?

Done.

[Nicolas Zea, 2016-01-04 23:34:56]

lgtm

https://codereview.chromium.org/1553433002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc
(right):

https://codereview.chromium.org/1553433002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc:102:
ASSERT_LT(recovery_time, base::TimeDelta::FromSeconds(2));
nit: Prefer using the test timeout constants (see test_timeouts.h, 
action_timeout is probably what you want)

[pavely, 2016-01-06 00:58:52]

Patchset #4 (id:60001) has been deleted

[pavely, 2016-01-06 01:03:53]

On 2016/01/04 23:34:56, Nicolas Zea wrote:
> lgtm
> 
>
https://codereview.chromium.org/1553433002/diff/40001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc
> (right):
> 
>
https://codereview.chromium.org/1553433002/diff/40001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc:102:
> ASSERT_LT(recovery_time, base::TimeDelta::FromSeconds(2));
> nit: Prefer using the test timeout constants (see test_timeouts.h, 
> action_timeout is probably what you want)

The thing is that action_timeout is at least 10 seconds, comparing recovery
interval with it I won't be able to tell if canary job ran immediately or
another retry happened.

[pavely, 2016-01-06 01:03:58]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-01-06 01:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1553433002/#ps80001
(title: "Fix android test")

[commit-bot: I haz the power, 2016-01-06 01:04:33]

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

[commit-bot: I haz the power, 2016-01-06 01:53:47]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-06 01:54:32]

Description was changed from

==========
[Sync] Sync should run canary cycle after any network related error

Today sync only runs canary cycle after small subset of network errors
that are related to connection/network failure. This leaves sync in
exponential backoff when unexpected error is returned from network
stack.

In this change I make sync translate all network errors to
CONNECTION_UNAVAILABLE therefore making it try canary job after network
change.

BUG=539943
R=zea@chromium.org
==========

to

==========
[Sync] Sync should run canary cycle after any network related error

Today sync only runs canary cycle after small subset of network errors
that are related to connection/network failure. This leaves sync in
exponential backoff when unexpected error is returned from network
stack.

In this change I make sync translate all network errors to
CONNECTION_UNAVAILABLE therefore making it try canary job after network
change.

BUG=539943
R=zea@chromium.org

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

[commit-bot: I haz the power, 2016-01-06 01:54:33]

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