[Sync] Sync types never recovers from throttle

[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644
blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.

Review-Url: https://codereview.chromium.org/2828423002
Cr-Commit-Position: refs/heads/master@{#468522}
Committed: https://chromium.googlesource.com/chromium/src/+/1d37839e54566033090fbb82f89dd1dba04c3621

[Gang Wu, 2017-04-20 19:24:05]

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

[commit-bot: I haz the power, 2017-04-20 19:24:55]

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

[Gang Wu, 2017-04-20 19:28:47]

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

[commit-bot: I haz the power, 2017-04-20 19:29:52]

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

[Gang Wu, 2017-04-20 19:31:45]

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

[commit-bot: I haz the power, 2017-04-20 19:32:40]

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, 2017-04-20 19:36:37]

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

[commit-bot: I haz the power, 2017-04-20 19:36:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Gang Wu, 2017-04-20 20:31:01]

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

[commit-bot: I haz the power, 2017-04-20 20:31:40]

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

[Gang Wu, 2017-04-20 20:31:56]

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

[commit-bot: I haz the power, 2017-04-20 20:33:46]

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

[Gang Wu, 2017-04-20 20:34:58]

Description was changed from

==========
Add unittests

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

to

==========
[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644

blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.
==========

[Gang Wu, 2017-04-20 20:34:59]

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

[Gang Wu, 2017-04-20 20:35:52]

Patchset #3 (id:40001) has been deleted

[Gang Wu, 2017-04-20 20:36:51]

Patchset #2 (id:20001) has been deleted

[Gang Wu, 2017-04-20 20:37:27]

Patchset #1 (id:1) has been deleted

[Gang Wu, 2017-04-20 20:38:48]

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

[commit-bot: I haz the power, 2017-04-20 20:39:11]

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

[Gang Wu, 2017-04-20 20:45:16]

Patchset #1 (id:60001) has been deleted

[Gang Wu, 2017-04-20 20:45:33]

Patchset #1 (id:80001) has been deleted

[Gang Wu, 2017-04-20 20:48:58]

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

[commit-bot: I haz the power, 2017-04-20 20:49:42]

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

[Gang Wu, 2017-04-20 21:56:19]

PTAL

[Gang Wu, 2017-04-20 21:56:32]

Description was changed from

==========
[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644

blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.
==========

to

==========
[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644


blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.
==========

[commit-bot: I haz the power, 2017-04-20 22:10:44]

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

[commit-bot: I haz the power, 2017-04-20 22:10:44]

Dry run: This issue passed the CQ dry run.

[skym, 2017-04-21 16:06:35]

I believe you that your changes fix the bug. But, this class already seems
confusing and fragile, and I worry these changes are just making it more so. I
think we need to try really hard to simplify things.

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:624: // Global throttling or
backoff
Can you add a comment explaining why you overwrite existing
pending_wakeup_timer_ jobs. Also punctuation :)

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:642: TimeTicks
incoming_run_time = TimeTicks::Now() + time_until_next_unblock;
I think this logic already exists in this file, maybe refactor into a helper
method?

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:742:
DCHECK(CalledOnValidThread());
Can you audit these CalledOnValidThread()? I think they should definitely be in
public methods, which we're missing in some places. Also, I think you could
argue we want them in methods that we pass as references outside of this class,
and I think you could argue we shouldn't (one shot timer always calls on current
task runner). Lets be consistent though. This method checks, PerformDelayedNudge
does not.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:752: RestartWaiting();
Sprinkling RestartWaiting()s all over the place, and then when the next
developer comes in here and makes changes and needs to update things... seems
really error prone. I think it'd be better if the pending_wakeup_timer_ was
always scheduled with the same method, that has a priority curried into it, and
only it needs to call RestartWaiting() at the end.

I'd also like to see the whole block that calls RestartWaiting removed from
SyncSchedulerImpl::TrySyncCycleJobImpl(). Yes, changes can jump in between
posting and running TrySyncCycleJobImpl(), but it shouldn't be responsible for
reacting to these kinds of things.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
This function makes no sense to me. I am unable to follow it. Why doesn't
unblock_time_ get reset if we're now is past unblock_time_ but we're also in
exponential backoff?

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:767: RestartWaiting();
Why isn't RestartWaiting() the last thing done in this function?

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:783: RestartWaiting();
Why isn't RestartWaiting() the last thing done in this function?

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:927:
retry_timer_.Start(FROM_HERE, delay, this,
Why do we have a separate timer for this? Why is this not also using the same
shared timer?

[Gang Wu, 2017-04-25 00:08:19]

Patchset #3 (id:140001) has been deleted

[Gang Wu, 2017-04-25 00:08:29]

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

[commit-bot: I haz the power, 2017-04-25 00:08:56]

Dry run: CQ is trying da patch.

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

[Gang Wu, 2017-04-25 00:52:06]

updated.

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:624: // Global throttling or
backoff
On 2017/04/21 16:06:34, skym wrote:
> Can you add a comment explaining why you overwrite existing
> pending_wakeup_timer_ jobs. Also punctuation :)

Done.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:642: TimeTicks
incoming_run_time = TimeTicks::Now() + time_until_next_unblock;
On 2017/04/21 16:06:34, skym wrote:
> I think this logic already exists in this file, maybe refactor into a helper
> method?

Done.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:742:
DCHECK(CalledOnValidThread());
On 2017/04/21 16:06:34, skym wrote:
> Can you audit these CalledOnValidThread()? I think they should definitely be
in
> public methods, which we're missing in some places. Also, I think you could
> argue we want them in methods that we pass as references outside of this
class,
> and I think you could argue we shouldn't (one shot timer always calls on
current
> task runner). Lets be consistent though. This method checks,
PerformDelayedNudge
> does not.

Done.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:752: RestartWaiting();
On 2017/04/21 16:06:34, skym wrote:
> Sprinkling RestartWaiting()s all over the place, and then when the next
> developer comes in here and makes changes and needs to update things... seems
> really error prone. I think it'd be better if the pending_wakeup_timer_ was
> always scheduled with the same method, that has a priority curried into it,
and
> only it needs to call RestartWaiting() at the end.
> 
> I'd also like to see the whole block that calls RestartWaiting removed from
> SyncSchedulerImpl::TrySyncCycleJobImpl(). Yes, changes can jump in between
> posting and running TrySyncCycleJobImpl(), but it shouldn't be responsible for
> reacting to these kinds of things.

Done.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2017/04/21 16:06:34, skym wrote:
> This function makes no sense to me. I am unable to follow it. Why doesn't
> unblock_time_ get reset if we're now is past unblock_time_ but we're also in
> exponential backoff?

Each datatype may have different backoff time, for example, BOOKMARK may be got
throttled at 1pm for 2 hours, and SESSION bay got throttled at 2pm for 3 hours.
So when time goes to 3pm, also BOOKMARK will be unblocked, but SESSIONS is
blocked still, and need to schedule another unblock job for SESSIONS.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:767: RestartWaiting();
On 2017/04/21 16:06:34, skym wrote:
> Why isn't RestartWaiting() the last thing done in this function?

Done.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:783: RestartWaiting();
On 2017/04/21 16:06:34, skym wrote:
> Why isn't RestartWaiting() the last thing done in this function?

Where to run RestartWaiting() is not a matter, we just schedule timer
somewhere is fine.
Anyway, I remove RestartWaiting() from all place and put at the end of
TrySyncCycleJobImpl.

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:927:
retry_timer_.Start(FROM_HERE, delay, this,
On 2017/04/21 16:06:34, skym wrote:
> Why do we have a separate timer for this? Why is this not also using the same
> shared timer?

There are a lot of logic here.
poll_timer_ is for poll jobs, which will run every 8 or 12 hours, or
defined by server.
retry_timer_ is for retry jobs, which is set by server. when client received a
command from server, client will schedule a job for it.

There are three of timers, and a lot of logic here, I do not think I can
refactor them in
this CL, so I create a TODO for it.

[commit-bot: I haz the power, 2017-04-25 00:52:54]

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

[commit-bot: I haz the power, 2017-04-25 00:52:55]

Dry run: This issue passed the CQ dry run.

[skym, 2017-04-25 23:16:17]

Sorry this took me so long to review.

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2017/04/25 00:52:05, Gang Wu wrote:
> On 2017/04/21 16:06:34, skym wrote:
> > This function makes no sense to me. I am unable to follow it. Why doesn't
> > unblock_time_ get reset if we're now is past unblock_time_ but we're also in
> > exponential backoff?
> 
> Each datatype may have different backoff time, for example, BOOKMARK may be
got
> throttled at 1pm for 2 hours, and SESSION bay got throttled at 2pm for 3
hours.
> So when time goes to 3pm, also BOOKMARK will be unblocked, but SESSIONS is
> blocked still, and need to schedule another unblock job for SESSIONS. 

Yes, but |unblock_time_| is inside of DataTypeTracker, it's already scoped to a
single data type.

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

https://codereview.chromium.org/2828423002/diff/160001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:659:
base::Bind(&SyncSchedulerImpl::OnTypesUnblocked,
This doesn't seem safe. If someone jumps in before this timer goes off and
messes with our internal state, OnTypesUnblocked never calls TrySyncCycleJob,
and our pending_wakeup_timer_ chain is broken.

I'd feel a lot safer if only 1 method ever ran on pending_wakeup_timer_.

https://codereview.chromium.org/2828423002/diff/160001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:724:
DCHECK(IsCurrentlyThrottled() || IsBackingOff() ||
How certain are you we're not going to start hitting this?

[Gang Wu, 2017-04-28 21:58:28]

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

[commit-bot: I haz the power, 2017-04-28 21:59:11]

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, 2017-04-28 22:27:42]

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

[commit-bot: I haz the power, 2017-04-28 22:27:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Gang Wu, 2017-04-28 23:58:25]

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

[commit-bot: I haz the power, 2017-04-28 23:59:06]

Dry run: CQ is trying da patch.

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

[Gang Wu, 2017-04-28 23:59:48]

Patchset #4 (id:180001) has been deleted

[Gang Wu, 2017-04-28 23:59:56]

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

[commit-bot: I haz the power, 2017-04-29 00:00:29]

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, 2017-04-29 01:45:03]

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

[commit-bot: I haz the power, 2017-04-29 01:45:04]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-04-29 15:06:20]

updated, PTAL

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2017/04/25 23:16:17, skym wrote:
> On 2017/04/25 00:52:05, Gang Wu wrote:
> > On 2017/04/21 16:06:34, skym wrote:
> > > This function makes no sense to me. I am unable to follow it. Why doesn't
> > > unblock_time_ get reset if we're now is past unblock_time_ but we're also
in
> > > exponential backoff?
> > 
> > Each datatype may have different backoff time, for example, BOOKMARK may be
> got
> > throttled at 1pm for 2 hours, and SESSION bay got throttled at 2pm for 3
> hours.
> > So when time goes to 3pm, also BOOKMARK will be unblocked, but SESSIONS is
> > blocked still, and need to schedule another unblock job for SESSIONS. 
> 
> Yes, but |unblock_time_| is inside of DataTypeTracker, it's already scoped to
a
> single data type.

You mean something like BOOKMARK got in throttling and exponential backoff? no,
that is not gonna happen. Once a datatype got in one of status of throttling or
backoff, client will not sent any update for that datatype, then the datatype
will not receive another error status.

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

https://codereview.chromium.org/2828423002/diff/160001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:659:
base::Bind(&SyncSchedulerImpl::OnTypesUnblocked,
On 2017/04/25 23:16:17, skym wrote:
> This doesn't seem safe. If someone jumps in before this timer goes off and
> messes with our internal state, OnTypesUnblocked never calls TrySyncCycleJob,
> and our pending_wakeup_timer_ chain is broken.
> 
> I'd feel a lot safer if only 1 method ever ran on pending_wakeup_timer_.

good catch, fixed by add an else in OnTypesUnblocked.

https://codereview.chromium.org/2828423002/diff/160001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:724:
DCHECK(IsCurrentlyThrottled() || IsBackingOff() ||
On 2017/04/25 23:16:17, skym wrote:
> How certain are you we're not going to start hitting this?

If we are in global backoff or throttling, and then schedule a nudge job, we
will hit this. otherwise it will go through CanRunNudgeJobNow(priority).

[skym, 2017-05-01 17:59:14]

lgtm

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
No, I'm saying that the code checks |unblock_time_| for both exponential backoff
and throttling cases, but it only clears it for throttling. Why does it clear it
for (and only for) throttling?

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

https://codereview.chromium.org/2828423002/diff/200001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:633: // Since
RestartWaiting() is called in TrySyncCycleJobImpl(), we should
I don't understand what this comment is saying, or why we should be checking
IsEarlierThanCurrentPendingJob for global blocking reasons.

https://codereview.chromium.org/2828423002/diff/200001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:728: RestartWaiting();
We should explain somewhere what the overall strategy here is. How we ensure we
call RestartWaiting() at the correct times and don't drop various throttling
scenarios like we used to. Maybe this explanation belongs in the .h file
definition of RestartWaiting()?

[Gang Wu, 2017-05-01 20:09:00]

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

[commit-bot: I haz the power, 2017-05-01 20:09:26]

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, 2017-05-01 21:30:29]

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

[commit-bot: I haz the power, 2017-05-01 21:30:31]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-05-01 23:01:41]

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

https://codereview.chromium.org/2828423002/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:764:
nudge_tracker_.UpdateTypeThrottlingAndBackoffState();
On 2017/05/01 17:59:14, skym wrote:
> No, I'm saying that the code checks |unblock_time_| for both exponential
backoff
> and throttling cases, but it only clears it for throttling. Why does it clear
it
> for (and only for) throttling?

oh, I see. we cannot clear for backoff case since if we clear backoff state, it
is hard to exponential. 
Throttling is cleared because throttling is one time thing, so un-throttling
every time when throttling is expired.

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

https://codereview.chromium.org/2828423002/diff/200001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:633: // Since
RestartWaiting() is called in TrySyncCycleJobImpl(), we should
On 2017/05/01 17:59:14, skym wrote:
> I don't understand what this comment is saying, or why we should be checking
> IsEarlierThanCurrentPendingJob for global blocking reasons.

Done.

https://codereview.chromium.org/2828423002/diff/200001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.cc:728: RestartWaiting();
On 2017/05/01 17:59:14, skym wrote:
> We should explain somewhere what the overall strategy here is. How we ensure
we
> call RestartWaiting() at the correct times and don't drop various throttling
> scenarios like we used to. Maybe this explanation belongs in the .h file
> definition of RestartWaiting()?

Done.

[Gang Wu, 2017-05-01 23:57:07]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2017-05-01 23:57:07]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2828423002/#ps220001
(title: "update comments")

[commit-bot: I haz the power, 2017-05-01 23:57:45]

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-02 00:06:52]

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

[commit-bot: I haz the power, 2017-05-02 00:06:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gang Wu, 2017-05-02 00:12:14]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2017-05-02 00:12:15]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2828423002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-02 00:12:53]

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-02 01:13:39]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493683934830190,
"parent_rev": "95b984c544b0216c36a04231151a6509ac7240d5", "commit_rev":
"1d37839e54566033090fbb82f89dd1dba04c3621"}

[commit-bot: I haz the power, 2017-05-02 01:13:57]

Description was changed from

==========
[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644


blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.
==========

to

==========
[Sync] Sync types never recovers from throttle

Should check if SyncSchedulerImpl::OnTypesUnblocked need to be scheduled.

BUG=710644


blocked jobs are 
SyncSchedulerImpl::Unthrottle
SyncSchedulerImpl::ExponentialBackoffRetry
SyncSchedulerImpl::OnTypesUnblocked

|pending_wakeup_timer_| is used to schedule blocked type jobs and
|PerformDelayedNudge|, and if OnTypesUnblocked is scheduled, but
|PerformDelayedNudge| or other blocked jobs got scheduled before
OnTypesUnblocked, OnTypesUnblocked will be replaced, and never been
scheduled again. so we should check if there are any type is blocked
after each of those jobs' run.

Review-Url: https://codereview.chromium.org/2828423002
Cr-Commit-Position: refs/heads/master@{#468522}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d37839e54566033090fbb82f89d...
==========

[commit-bot: I haz the power, 2017-05-02 01:13:58]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/1d37839e54566033090fbb82f89d...