sync: raise initial backoff interval from 1 second to 5 minutes.

sync: raise initial backoff interval from 1 second to 5 minutes.

Long term, we'd like to drop this further (i.e. 1 minute) and trust THROTTLED responses in the event of widespread backend outages.
We'd like to run some stress tests of that infrastructure (at scale / under load), but in the meantime we want to address the problem
for beta channel users / M22.  We chose 5 minutes, which is a little more aggressive than the recommended value, but we believe is
safe -- happy to discuss offline.

This patch is very low risk (stability wise). although 5 minutes is a long time to wait for users who hit this due to transient failures.
Current data suggests 500s occur around 0.02% of the time, and although backoff isn't limited to 500s it's unlikely to get hit frequently,
and the user experience doesn't break down significantly.

Effectiveness wise, We can run an experiment once it hits dev channel by returning 500s and watching for expected drops in QPS.

BUG=139733, 142029
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151216

[rlarocque, 2012-08-08 01:58:23]

Reluctant LGTM.  I think this is a bit too conservative, but have no way of
showing that any lower interval is actually safe.

Hopefully the "nudge during backoff interval" will help hide the long backoff
interval.

http://codereview.chromium.org/10826194/diff/1/sync/engine/sync_scheduler_imp...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10826194/diff/1/sync/engine/sync_scheduler_imp...
sync/engine/sync_scheduler_impl.cc:910: IsBackingOff() ? wait_interval_->length
: TimeDelta::FromMinutes(5));
While you're changing this, maybe it should be moved to polling_constants.h?

[tim (not reviewing), 2012-08-08 05:57:04]

http://codereview.chromium.org/10826194/diff/1/sync/engine/sync_scheduler_imp...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10826194/diff/1/sync/engine/sync_scheduler_imp...
sync/engine/sync_scheduler_impl.cc:910: IsBackingOff() ? wait_interval_->length
: TimeDelta::FromMinutes(5));
On 2012/08/08 01:58:23, rlarocque wrote:
> While you're changing this, maybe it should be moved to polling_constants.h?

I actually want to do that, but I want to keep this as tiny as possible
particularly for TPMs since we'll want to merge to beta on very short notice. 
I'll prep a follow up.

[tim (not reviewing), 2012-08-10 23:11:23]

Two changes.

1 - treat MIGRATION_DONE specially (keep 1s retry).

2 - Added egregious temporarily poke at static method on SyncSchedulerImpl from
integration tests to override exponential backoff delay, and avoid timing out
the tests.

I'm filing a bug (and have TODOs all over) to clean this up and use a flag
that's plumbed through, but keeping as short as possible for M22 merging.

PTAL

[rlarocque, 2012-08-11 00:22:56]

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
File sync/engine/sync_scheduler_impl.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:38: static bool g_force_immediate_retry =
false;
Discussed offline.  I'll look forward to reviewing the patch that removes this
hack.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:919: TimeDelta
SyncSchedulerImpl::GetInitialBackoffDelay(
Although I don't know what the "right" place to put this code would be, I'm
certain that this is the wrong place for it.

This function accesses no non-static SyncSchedulerImpl members.  It's only
sort-of related to scheduling.  

I'm not sure what class this should belong to.  Maybe SyncSession? 
DelayProvider?  

Making it file-static would be not great, but it would be an improvement over
the current definition.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:935: state.commit_result ==
SERVER_RETURN_MIGRATION_DONE) {
What's the thinking behind checking the commit's return value?  According to the
comment above (which I agree with) this will never matter.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
File sync/engine/sync_scheduler_unittest.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_unittest.cc:923: TEST_F(SyncSchedulerTest,
BackoffElevation) {
This test might be evidence in favor of moving the initial backoff calculation
into DelayProvider.  That would make the testing of the first backoff interval
slightly less special.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/internal_api/p...
File sync/internal_api/public/engine/polling_constants.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/internal_api/p...
sync/internal_api/public/engine/polling_constants.cc:32: const int
kInitialBackoffImmediateRetrySeconds = 1;
The word "Immediate" is misleading here.  Maybe
s/BackoffImmediate/ShortBackoff/?

[tim (not reviewing), 2012-08-11 00:43:58]

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
File sync/engine/sync_scheduler_impl.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:919: TimeDelta
SyncSchedulerImpl::GetInitialBackoffDelay(
On 2012/08/11 00:22:56, rlarocque wrote:
> Although I don't know what the "right" place to put this code would be, I'm
> certain that this is the wrong place for it.
> 
> This function accesses no non-static SyncSchedulerImpl members.  It's only
> sort-of related to scheduling.  
> 
> I'm not sure what class this should belong to.  Maybe SyncSession? 
> DelayProvider?  
> 
> Making it file-static would be not great, but it would be an improvement over
> the current definition.

Not sure I agree with that.  Making it file-static makes it harder to test, for
one.

SyncSession should not know / care about scheduling related times and delays.

The scheduler is the thing responsible for controlling all things backoff
related, so I don't feel this is that out of place. It currently does the
GetRecommendedDelay math, too, although this could go in DelayProvider if we
wanted. 

DelayProvider could work.  But making DelayProvider aware of different error
codes seems wrong. The only thing that really knows about sync protocol errors
and exponential backoff is the scheduler... if we moved all backoff control out
of SyncSchedulerImpl into a BackoffController class... I could see that
working...

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:935: state.commit_result ==
SERVER_RETURN_MIGRATION_DONE) {
On 2012/08/11 00:22:56, rlarocque wrote:
> What's the thinking behind checking the commit's return value?  According to
the
> comment above (which I agree with) this will never matter.

It is technically possible to get a migration done on commit, where you didn't
get one on download updates, right?  I suppose now that all users have been
migrated server side it isn't, since there won't be any changes in that state,
but if we ever re-use migration or do a different migration... seems like it's a
legal case?  Am I missing something?

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/internal_api/p...
File sync/internal_api/public/engine/polling_constants.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/internal_api/p...
sync/internal_api/public/engine/polling_constants.cc:32: const int
kInitialBackoffImmediateRetrySeconds = 1;
On 2012/08/11 00:22:56, rlarocque wrote:
> The word "Immediate" is misleading here.  Maybe
> s/BackoffImmediate/ShortBackoff/?

Ok.  I had QuickBackoff initially, but I thought it sounded too approachable and
wanted something that drove home the fact that you probably dont want this.  I
even toyed with kInitialBackoffDangerouslyQuickRetry :)  I guess I'll just go
with 'Short'.

[rlarocque, 2012-08-11 00:58:33]

LGTM.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
File sync/engine/sync_scheduler_impl.cc (right):

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:919: TimeDelta
SyncSchedulerImpl::GetInitialBackoffDelay(
On 2012/08/11 00:43:58, timsteele wrote:
> On 2012/08/11 00:22:56, rlarocque wrote:
> > Although I don't know what the "right" place to put this code would be, I'm
> > certain that this is the wrong place for it.
> > 
> > This function accesses no non-static SyncSchedulerImpl members.  It's only
> > sort-of related to scheduling.  
> > 
> > I'm not sure what class this should belong to.  Maybe SyncSession? 
> > DelayProvider?  
> > 
> > Making it file-static would be not great, but it would be an improvement
over
> > the current definition.
> 
> Not sure I agree with that.  Making it file-static makes it harder to test,
for
> one.
> 
> SyncSession should not know / care about scheduling related times and delays.
> 
> The scheduler is the thing responsible for controlling all things backoff
> related, so I don't feel this is that out of place. It currently does the
> GetRecommendedDelay math, too, although this could go in DelayProvider if we
> wanted. 
> 
> DelayProvider could work.  But making DelayProvider aware of different error
> codes seems wrong. The only thing that really knows about sync protocol errors
> and exponential backoff is the scheduler... if we moved all backoff control
out
> of SyncSchedulerImpl into a BackoffController class... I could see that
> working...

That last suggestion sounds reasonable, but it's probably outside the scope of
this patch.  Let's revisit this when you get around to writing the cleanup
patch.

https://chromiumcodereview.appspot.com/10826194/diff/2004/sync/engine/sync_sc...
sync/engine/sync_scheduler_impl.cc:935: state.commit_result ==
SERVER_RETURN_MIGRATION_DONE) {
On 2012/08/11 00:43:58, timsteele wrote:
> On 2012/08/11 00:22:56, rlarocque wrote:
> > What's the thinking behind checking the commit's return value?  According to
> the
> > comment above (which I agree with) this will never matter.
> 
> It is technically possible to get a migration done on commit, where you didn't
> get one on download updates, right?  I suppose now that all users have been
> migrated server side it isn't, since there won't be any changes in that state,
> but if we ever re-use migration or do a different migration... seems like it's
a
> legal case?  Am I missing something?

I suppose it's legal, but extremely unlikely.  Then again, it will become more
likely when we remove the pre-commit GetUpdate...

OK, I'm convinced.  Let's leave it in.

[commit-bot: I haz the power, 2012-08-11 01:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10826194/5003

[commit-bot: I haz the power, 2012-08-11 01:34:58]

Try job failure for 10826194-5003 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-12 23:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10826194/13002

[commit-bot: I haz the power, 2012-08-13 01:07:46]

Change committed as 151216