[Sync] Sync client should to exponential backoff when receive partial failure

[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072
Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set,
Sync client will throttle those datatypes, and also throttle whole client.
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly,
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).

3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.

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

[Gang Wu, 2016-11-03 18:55:48]

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

[commit-bot: I haz the power, 2016-11-03 18:56:47]

Dry run: 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-11-03 20:08:50]

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

[commit-bot: I haz the power, 2016-11-03 20:08:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2016-11-07 23:09:07]

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

[commit-bot: I haz the power, 2016-11-07 23:09:43]

Dry run: 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-11-07 23:19:45]

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

[commit-bot: I haz the power, 2016-11-07 23:19:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Gang Wu, 2016-11-08 07:37:44]

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

[commit-bot: I haz the power, 2016-11-08 07:38:00]

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

[Gang Wu, 2016-11-08 08:18:07]

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

[commit-bot: I haz the power, 2016-11-08 08:18:30]

Dry run: 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-11-08 09:39:38]

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

[commit-bot: I haz the power, 2016-11-08 09:39:39]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-11-08 16:12:32]

Patchset #2 (id:20001) has been deleted

[Gang Wu, 2016-11-08 16:18:30]

Patchset #2 (id:40001) has been deleted

[Gang Wu, 2016-11-08 17:58:01]

Patchset #1 (id:1) has been deleted

[Gang Wu, 2016-11-08 17:59:14]

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

[commit-bot: I haz the power, 2016-11-08 17:59:43]

Dry run: 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-11-08 18:57:45]

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

[commit-bot: I haz the power, 2016-11-08 18:57:46]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-11-08 19:00:31]

Description was changed from

==========
functional works, needs more tests


first compilable

BUG=
==========

to

==========
[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072

Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set, 
Sync client will throttle those datatypes, and also throttle whole client. 
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly, 
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).


3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.
==========

[Gang Wu, 2016-11-08 19:00:32]

gangwu@chromium.org changed reviewers:
+ zea@chromium.org

[Gang Wu, 2016-11-08 19:02:53]

Patchset #1 (id:60001) has been deleted

[Gang Wu, 2016-11-08 19:03:08]

PTAL

[Nicolas Zea, 2016-11-09 00:21:25]

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.cc:257: if (!IsBackedOff())
{
nit: this should probably just be DCHECK(IsBackedOff()). Same applies to
IsThrottled above. In general, we shouldn't be attempting to handle invalid
states (like you do by having an if statement). Or if we do handle them, they
shouldn't be triggering DCHECKs.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.cc:287: if (now >=
unblock_time_) {
I don't understand this method. Why is |now| a parameter? Should we just be
checking if unblock_time_ is in the past (e.g. compare against
base::TimeTicks::Now())?

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.h:132: base::TimeDelta
GetLastUnbackoffInterval() const;
Should "Unbackoff" be "Backoff"? I'm not sure what unbackoff interval would
mean.

I also wonder how interesting this value is, and whether we need to surface it
at all.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:563: if
(model_neutral_state.commit_result == SERVER_RETURN_PARTIAL_FAILURE) {
Should partial failure just count as success?

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:796: void
SyncSchedulerImpl::TypesUnblock(TimeTicks unbackoff_time) {
I think naming this method something like OnTypesUnblocked would be clearer.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:802: // If there are some
datatype still throttling or backoff, find the closest
"still throttling or backoff" -> "still throttled or backed off"

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:817: now +
time_until_next_unblock));
as mentioned earlier, it's not clear to me why we need to pass an argument to
TypesUnlock. Given the timer will call it at the right time, we should I think
be able to rely on base::TimeTicks::Now()

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:881:
type_wakeup_timer_.Start(FROM_HERE, time_until_next_unthrottle,
Why do we need a separate timer? Can't we reuse the global timer?

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/syncer_proto_util.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/syncer_proto_util.cc:475: // This is a spicial case,
since server only throttle some of datatype,
typo: spicial -> special

[Gang Wu, 2016-11-10 18:54:37]

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

[commit-bot: I haz the power, 2016-11-10 18:54:59]

Dry run: 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-11-10 20:00:36]

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

[commit-bot: I haz the power, 2016-11-10 20:00:37]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-11-10 21:56:51]

Merged two timer to one timer.
directly use Now() instead of pass it as parameter.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.cc:257: if (!IsBackedOff())
{
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> nit: this should probably just be DCHECK(IsBackedOff()). Same applies to
> IsThrottled above. In general, we shouldn't be attempting to handle invalid
> states (like you do by having an if statement). Or if we do handle them, they
> shouldn't be triggering DCHECKs.

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.cc:287: if (now >=
unblock_time_) {
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> I don't understand this method. Why is |now| a parameter? Should we just be
> checking if unblock_time_ is in the past (e.g. compare against
> base::TimeTicks::Now())?

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/cycle/data_type_tracker.h:132: base::TimeDelta
GetLastUnbackoffInterval() const;
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> Should "Unbackoff" be "Backoff"? I'm not sure what unbackoff interval would
> mean.
> 
> I also wonder how interesting this value is, and whether we need to surface it
> at all.

This is for SyncSchedulerImpl to exponential the backoff interval.
If do not do this way, then we need to pass DelayProvider to here to do the
exponential.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:563: if
(model_neutral_state.commit_result == SERVER_RETURN_PARTIAL_FAILURE) {
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> Should partial failure just count as success?

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:796: void
SyncSchedulerImpl::TypesUnblock(TimeTicks unbackoff_time) {
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> I think naming this method something like OnTypesUnblocked would be clearer.

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:802: // If there are some
datatype still throttling or backoff, find the closest
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> "still throttling or backoff" -> "still throttled or backed off"

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:817: now +
time_until_next_unblock));
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> as mentioned earlier, it's not clear to me why we need to pass an argument to
> TypesUnlock. Given the timer will call it at the right time, we should I think
> be able to rely on base::TimeTicks::Now()

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/sync_scheduler_impl.cc:881:
type_wakeup_timer_.Start(FROM_HERE, time_until_next_unthrottle,
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> Why do we need a separate timer? Can't we reuse the global timer?

Done.

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/syncer_proto_util.cc (right):

https://codereview.chromium.org/2475043002/diff/80001/components/sync/engine_...
components/sync/engine_impl/syncer_proto_util.cc:475: // This is a spicial case,
since server only throttle some of datatype,
On 2016/11/09 00:21:25, Nicolas Zea wrote:
> typo: spicial -> special

Done.

[Nicolas Zea, 2016-11-10 23:28:22]

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:37:
WaitInterval(BlockingMode mode, base::TimeDelta length);
nit: move this constructor above the destructor

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:119: bool IsBackedOff()
const;
Does it make sense to combine IsThrottled and IsBackedOff into a single
IsBlocked method? As far as I can tell, any callsite that cares about
IsThrottled should also be checking IsBackedOff, so may as well combine the two.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:133: // Backoffs the type
from |now| until |now| + |duration|.
nit: "backoffs the type" -> "Backs off the type"

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:134: void
BackoffType(base::TimeDelta duration, base::TimeTicks now);
nit: rename to BackOffType

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:136: // Unthrottles or
Unbackoffs the type if base::TimeTicks::Now() >=
Unbackoff isn't really a word. Perhaps rephrase "Unthrottles or unbackoffs the
tyep" to "Unblocks the type"

That said, I'm a bit concerned about this. It makes sense to unthrottle after
the time is up, but backoff should only end only a successful response.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:801:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
See my other comment about this method. I think this is going to reset backoff
state too easily.

Consider for example:
Sync request bookmarks, and receives a backoff response.
Sync waits X mins. Backoff state is reset
After that time, sync requests bookmarks again, and receives the same backoff
response.
Sync will only wait X mins.

This basically means that backoff is behaviorally the same as throttling, right?
We don't want to reset backoff state just to retry. We should only reset backoff
state on success.

[Gang Wu, 2016-11-11 18:18:22]

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

[commit-bot: I haz the power, 2016-11-11 18:18:56]

Dry run: 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-11-11 19:10:59]

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

[commit-bot: I haz the power, 2016-11-11 19:11:00]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-11-11 19:15:31]

fixed backoff issue.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:37:
WaitInterval(BlockingMode mode, base::TimeDelta length);
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> nit: move this constructor above the destructor

Done.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:119: bool IsBackedOff()
const;
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> Does it make sense to combine IsThrottled and IsBackedOff into a single
> IsBlocked method? As far as I can tell, any callsite that cares about
> IsThrottled should also be checking IsBackedOff, so may as well combine the
two.

Done.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:133: // Backoffs the type
from |now| until |now| + |duration|.
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> nit: "backoffs the type" -> "Backs off the type"

Done.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:134: void
BackoffType(base::TimeDelta duration, base::TimeTicks now);
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> nit: rename to BackOffType

Done.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:136: // Unthrottles or
Unbackoffs the type if base::TimeTicks::Now() >=
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> Unbackoff isn't really a word. Perhaps rephrase "Unthrottles or unbackoffs the
> tyep" to "Unblocks the type"
> 
> That said, I'm a bit concerned about this. It makes sense to unthrottle after
> the time is up, but backoff should only end only a successful response.

Yes, fixed by add another blocking mode which is EXPONENTIAL_BACKOFF_RETRYING.

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/2475043002/diff/100001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:801:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2016/11/10 23:28:21, Nicolas Zea wrote:
> See my other comment about this method. I think this is going to reset backoff
> state too easily.
> 
> Consider for example:
> Sync request bookmarks, and receives a backoff response.
> Sync waits X mins. Backoff state is reset
> After that time, sync requests bookmarks again, and receives the same backoff
> response.
> Sync will only wait X mins.
> 
> This basically means that backoff is behaviorally the same as throttling,
right?
> We don't want to reset backoff state just to retry. We should only reset
backoff
> state on success.

Yes, just added another blocking state, EXPONENTIAL_BACKOFF_RETRYING

[Nicolas Zea, 2016-11-12 01:04:35]

Do you have any tests that ensure the backoff grows with repeated failures, and
then gets reset on success?

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:275: wait_interval_->mode
= WaitInterval::EXPONENTIAL_BACKOFF_RETRYING;
When will a type exit EXPONENTIAL_BACKOFF_RETRYING state? As far as I can tell
it will only exit when the timer for a _different_ type goes off, which seems
unreliable. Presumably we should ensure the wait interval gets reset reliably on
successful sync?

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/cycle/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker_unittest.cc:474:
nudge_tracker_.RecordSuccessfulSyncCycle();
I find it confusing that recording a successful sync cycle has no impact on
backoff state (similar to my other comment). Perhaps a successful sync cycle
should be the trigger for resetting BACKOFF_RETRY?

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker_unittest.cc:572:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
Would be good to verify the retry logic as well.

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/syncer_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/syncer_unittest.cc:1026:
EXPECT_TRUE(SyncShareNudge());
Shouldn't bookmarks be blocked when this happens?

[Gang Wu, 2016-11-14 18:45:09]

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

[commit-bot: I haz the power, 2016-11-14 18:46:30]

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

[Gang Wu, 2016-11-14 19:14:51]

exponential backoff test added

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:275: wait_interval_->mode
= WaitInterval::EXPONENTIAL_BACKOFF_RETRYING;
On 2016/11/12 01:04:35, Nicolas Zea wrote:
> When will a type exit EXPONENTIAL_BACKOFF_RETRYING state? As far as I can tell
> it will only exit when the timer for a _different_ type goes off, which seems
> unreliable. Presumably we should ensure the wait interval gets reset reliably
on
> successful sync?

It will get reset on successful sync on
DataTypeTracker::RecordSuccessfulSyncCycle(line 134)

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/cycle/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker_unittest.cc:474:
nudge_tracker_.RecordSuccessfulSyncCycle();
On 2016/11/12 01:04:35, Nicolas Zea wrote:
> I find it confusing that recording a successful sync cycle has no impact on
> backoff state (similar to my other comment). Perhaps a successful sync cycle
> should be the trigger for resetting BACKOFF_RETRY?

We did not directly reset backoff state in
NudgeTracker::RecordSuccessfulSyncCycle, but we reset backoff state in
DataTypeTracker::RecordSuccessfulSyncCycle.

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker_unittest.cc:572:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2016/11/12 01:04:35, Nicolas Zea wrote:
> Would be good to verify the retry logic as well.

Done.

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/syncer_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/syncer_unittest.cc:1026:
EXPECT_TRUE(SyncShareNudge());
On 2016/11/12 01:04:35, Nicolas Zea wrote:
> Shouldn't bookmarks be blocked when this happens?

Partial failure does not block whole sync cycle, and is counted as success.
So SyncShareNudge() returning true means the whole sync cycle success,  line
1030-1032 will verify there are entries unsynced.

BTW, this test is copied from previous test GetUpdatesPartialThrottled.

[commit-bot: I haz the power, 2016-11-14 20:06:00]

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

[commit-bot: I haz the power, 2016-11-14 20:06:01]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2016-11-14 21:57:15]

Mostly LG with a couple cleanups/clarifications

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
File components/sync/engine_impl/cycle/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/120001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker_unittest.cc:474:
nudge_tracker_.RecordSuccessfulSyncCycle();
On 2016/11/14 19:14:51, Gang Wu wrote:
> On 2016/11/12 01:04:35, Nicolas Zea wrote:
> > I find it confusing that recording a successful sync cycle has no impact on
> > backoff state (similar to my other comment). Perhaps a successful sync cycle
> > should be the trigger for resetting BACKOFF_RETRY?
> 
> We did not directly reset backoff state in
> NudgeTracker::RecordSuccessfulSyncCycle, but we reset backoff state in
> DataTypeTracker::RecordSuccessfulSyncCycle.
> 

I see, so if its blocked this does nothing, but if it's retrying this resets.
Makes sense.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:133: // reset throtling
and backoff state if state is EXPONENTIAL_BACKOFF_RETRYING
nit: reset throtling -> Reset throttling. Also, you can just remove the "if
state is...", given you're resetting unconditionally.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:78: void
RecordSuccessfulSyncCycle();
Would be good to add a comment here explaining how this relates to
throttled/backed off types.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/nudge_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker.cc:247: DCHECK(tracker_it !=
type_trackers_.end());
nit: DCHECK_NE?

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:274: return
tracker_it->second->GetTimeUntilUnblock();
I think it would be better to query the wait interval time directly, rather than
time until unblock.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:1719: // The
Exponential backoff should between previous backoff 1.5 and 2.5 times.
"should between" -> "should be between"

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:1723:
EXPECT_LE(first_blocking_time * 1.4, second_blocking_time);
If you query the interval time, rather than time until unblock, you should be
better able to verify that it has grown exponentially.

[Gang Wu, 2016-11-14 23:29:47]

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

[commit-bot: I haz the power, 2016-11-14 23:31:28]

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

[Gang Wu, 2016-11-14 23:40:26]

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

[Gang Wu, 2016-11-14 23:40:42]

Patchset #5 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-11-14 23:42:00]

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

[Gang Wu, 2016-11-15 01:59:32]

updated.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:133: // reset throtling
and backoff state if state is EXPONENTIAL_BACKOFF_RETRYING
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> nit: reset throtling -> Reset throttling. Also, you can just remove the "if
> state is...", given you're resetting unconditionally.

Done.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.h (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.h:78: void
RecordSuccessfulSyncCycle();
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> Would be good to add a comment here explaining how this relates to
> throttled/backed off types.

Done.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/cycle/nudge_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/cycle/nudge_tracker.cc:247: DCHECK(tracker_it !=
type_trackers_.end());
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> nit: DCHECK_NE?

Cannot use DCHECK_NE, compiler will failed on logging.h:548, no known conversion
from '...const_iterator...' to  'std::nullptr_t'

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:274: return
tracker_it->second->GetTimeUntilUnblock();
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> I think it would be better to query the wait interval time directly, rather
than
> time until unblock.

Done.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:1719: // The
Exponential backoff should between previous backoff 1.5 and 2.5 times.
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> "should between" -> "should be between"

Done.

https://codereview.chromium.org/2475043002/diff/140001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl_unittest.cc:1723:
EXPECT_LE(first_blocking_time * 1.4, second_blocking_time);
On 2016/11/14 21:57:15, Nicolas Zea wrote:
> If you query the interval time, rather than time until unblock, you should be
> better able to verify that it has grown exponentially.

Done.

[commit-bot: I haz the power, 2016-11-15 02:07:19]

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

[commit-bot: I haz the power, 2016-11-15 02:07:21]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2016-11-15 02:28:58]

lgtm

https://codereview.chromium.org/2475043002/diff/180001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/180001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:128: // If we were
throttled, then we would have been excluded from this cycle's
nit: throttled -> blocked

[Gang Wu, 2016-11-15 02:31:32]

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

[commit-bot: I haz the power, 2016-11-15 02:32:48]

Dry run: 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-11-15 03:55:55]

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

[commit-bot: I haz the power, 2016-11-15 03:55:56]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-11-15 17:51:24]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-11-15 17:51:24]

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/2475043002/#ps200001
(title: "update comments")

[Gang Wu, 2016-11-15 17:51:39]

https://codereview.chromium.org/2475043002/diff/180001/components/sync/engine...
File components/sync/engine_impl/cycle/data_type_tracker.cc (right):

https://codereview.chromium.org/2475043002/diff/180001/components/sync/engine...
components/sync/engine_impl/cycle/data_type_tracker.cc:128: // If we were
throttled, then we would have been excluded from this cycle's
On 2016/11/15 02:28:57, Nicolas Zea wrote:
> nit: throttled -> blocked

Done.

[commit-bot: I haz the power, 2016-11-15 17:52:00]

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-11-15 17:58:46]

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

[commit-bot: I haz the power, 2016-11-15 17:58:47]

Failed to apply patch for components/sync/engine_impl/sync_scheduler_impl.h:
While running git apply --index -p1;
  error: patch failed: components/sync/engine_impl/sync_scheduler_impl.h:116
  error: components/sync/engine_impl/sync_scheduler_impl.h: patch does not apply

Patch:       components/sync/engine_impl/sync_scheduler_impl.h
Index: components/sync/engine_impl/sync_scheduler_impl.h
diff --git a/components/sync/engine_impl/sync_scheduler_impl.h
b/components/sync/engine_impl/sync_scheduler_impl.h
index
7c5e06f825e4c3648b236b0f860f7de02fb1bdd6..061e78b608d5f32d5ccf9d98cfd0b49bc6ad07e6
100644
--- a/components/sync/engine_impl/sync_scheduler_impl.h
+++ b/components/sync/engine_impl/sync_scheduler_impl.h
@@ -70,6 +70,7 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   void OnThrottled(const base::TimeDelta& throttle_duration) override;
   void OnTypesThrottled(ModelTypeSet types,
                         const base::TimeDelta& throttle_duration) override;
+  void OnTypesBackedOff(ModelTypeSet types) override;
   bool IsCurrentlyThrottled() override;
   void OnReceivedShortPollIntervalUpdate(
       const base::TimeDelta& new_interval) override;
@@ -116,27 +117,6 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerTest, FailedRetry);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerTest, ReceiveNewRetryDelay);
 
-  struct WaitInterval {
-    enum Mode {
-      // Uninitialized state, should not be set in practice.
-      UNKNOWN = -1,
-      // We enter a series of increasingly longer WaitIntervals if we
experience
-      // repeated transient failures.  We retry at the end of each interval.
-      EXPONENTIAL_BACKOFF,
-      // A server-initiated throttled interval.  We do not allow any syncing
-      // during such an interval.
-      THROTTLED,
-    };
-    WaitInterval();
-    ~WaitInterval();
-    WaitInterval(Mode mode, base::TimeDelta length);
-
-    static const char* GetModeString(Mode mode);
-
-    Mode mode;
-    base::TimeDelta length;
-  };
-
   static const char* GetModeString(Mode mode);
 
   void SetDefaultNudgeDelay(base::TimeDelta delay_ms);
@@ -165,7 +145,7 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   // resets the poll interval, depedning on the flag's value.
   void AdjustPolling(PollAdjustType type);
 
-  // Helper to restart waiting with |wait_interval_|'s timer.
+  // Helper to restart pending_wakeup_timer_.
   void RestartWaiting();
 
   // Determines if we're allowed to contact the server right now.
@@ -184,8 +164,8 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   // Helper to signal listeners about changed retry time.
   void NotifyRetryTime(base::Time retry_time);
 
-  // Helper to signal listeners about changed throttled types.
-  void NotifyThrottledTypesChanged(ModelTypeSet types);
+  // Helper to signal listeners about changed throttled or backed off types.
+  void NotifyBlockedTypesChanged(ModelTypeSet types);
 
   // Looks for pending work and, if it finds any, run this work at "canary"
   // priority.
@@ -197,15 +177,17 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   void TrySyncCycleJobImpl();
 
   // Transitions out of the THROTTLED WaitInterval then calls TryCanaryJob().
+  // This function is for global throttling.
   void Unthrottle();
 
-  // Called when a per-type throttling interval expires.
-  void TypeUnthrottle(base::TimeTicks unthrottle_time);
+  // Called when a per-type throttling or backing off interval expires.
+  void OnTypesUnblocked();
 
   // Runs a normal nudge job when the scheduled timer expires.
   void PerformDelayedNudge();
 
   // Attempts to exit EXPONENTIAL_BACKOFF by calling TryCanaryJob().
+  // This function is for global backoff.
   void ExponentialBackoffRetry();
 
   // Called when the root cause of the current connection error is fixed.
@@ -217,8 +199,9 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   // Creates a cycle for a retry and performs the sync.
   void RetryTimerCallback();
 
-  // Returns the set of types that are enabled and not currently throttled.
-  ModelTypeSet GetEnabledAndUnthrottledTypes();
+  // Returns the set of types that are enabled and not currently throttled and
+  // backed off.
+  ModelTypeSet GetEnabledAndUnblockedTypes();
 
   // Called as we are started to broadcast an initial cycle snapshot
   // containing data like initial_sync_ended.  Important when the client starts
@@ -259,20 +242,16 @@ class SyncSchedulerImpl : public SyncScheduler, public
base::NonThreadSafe {
   std::unique_ptr<BackoffDelayProvider> delay_provider_;
 
   // The event that will wake us up.
+  // When the whole client got throttling or backoff, we will delay this timer
+  // as well.
   base::OneShotTimer pending_wakeup_timer_;
 
-  // An event that fires when data type throttling expires.
-  base::OneShotTimer type_unthrottle_timer_;
-
   // Storage for variables related to an in-progress configure request.  Note
   // that (mode_ != CONFIGURATION_MODE) \implies !pending_configure_params_.
   std::unique_ptr<ConfigurationParams> pending_configure_params_;
 
   std::unique_ptr<ClearParams> pending_clear_params_;
 
-  // If we have a nudge pending to run soon, it will be listed here.
-  base::TimeTicks scheduled_nudge_time_;
-
   // Keeps track of work that the syncer needs to handle.
   NudgeTracker nudge_tracker_;

[Gang Wu, 2016-11-15 18:30:39]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-11-15 18:30:40]

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/2475043002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-15 18:31:15]

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-11-15 20:56:30]

Description was changed from

==========
[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072

Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set, 
Sync client will throttle those datatypes, and also throttle whole client. 
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly, 
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).


3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.
==========

to

==========
[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072

Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set, 
Sync client will throttle those datatypes, and also throttle whole client. 
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly, 
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).


3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.
==========

[commit-bot: I haz the power, 2016-11-15 20:56:31]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-11-15 21:00:43]

Description was changed from

==========
[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072

Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set, 
Sync client will throttle those datatypes, and also throttle whole client. 
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly, 
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).


3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.
==========

to

==========
[Sync] Sync client should to exponential backoff when receive partial failure.

BUG=656072

Before this CL, Sync will throttle the data types in
error_data_type_ids, which is wrong, Sync should exponential backoff
those data type.

This CL also fix several bugs.
1. When Sync client receive error THROTTLED, and error_data_type_ids is set,
Sync client will throttle those datatypes, and also throttle whole client.
This is wrong, the whole client should not get throttled in this case.
To fix this, when this case happened, SyncerProtoUtil::PostClientToServerMessage
will throttled the data types, and return PARTIAL_FAILURE instead of THROTTLED.

2. |scheduled_nudge_time_| in SyncSchedulerImpl is not update correctly,
so Sync client will sync less frequently than expected.
To fix this, I delete |scheduled_nudge_time_| and get the time directly from
|global_wakeup_timer_| (previously pending_wakeup_timer_).

3. When Sync client received PARTIAL_FAILURE, not only the data type in
error_data_type_ids got throttled, the whole client got exponential backoff.
To fix this, add an early return in SyncSchedulerImpl::HandleFailure when
receive PARTIAL_FAILURE.

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

[commit-bot: I haz the power, 2016-11-15 21:00:45]

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