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

sync/engine/sync_scheduler_impl.cc

Index: sync/engine/sync_scheduler_impl.cc
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index ec7143de6a9cfc49946833c7173ff747586bad42..ff12d036db0ac087aaf613fd2d77b86b50e870d7 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -31,6 +31,12 @@ using sessions::SyncSourceInfo;
 using sync_pb::GetUpdatesCallerInfo;
 
 namespace {
+
+// For integration tests only.  Override initial backoff value.
+// TODO(tim): Remove this egregiousness, use command line flag and plumb
+// through. Done this way to reduce diffs in hotfix.
+static bool g_force_immediate_retry = false;

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

Discussed offline.  I'll look forward to reviewing the patch that removes this
hack.

+
 bool ShouldRequestEarlyExit(const SyncProtocolError& error) {
   switch (error.error_type) {
     case SYNC_SUCCESS:
@@ -897,6 +903,42 @@ void SyncSchedulerImpl::RestartWaiting() {
                               this, &SyncSchedulerImpl::DoCanaryJob);
 }
 
+namespace {
+// TODO(tim): Move this function to syncer_error.h.
+// Return true if the command in question was attempted and did not complete
+// successfully.
+bool IsError(SyncerError error) {
+  return error != UNSET && error != SYNCER_OK;
+}
+}  // namespace
+
+void SyncSchedulerImpl::ForceImmediateInitialBackoffRetry() {
+  g_force_immediate_retry = true;
+}
+
+TimeDelta SyncSchedulerImpl::GetInitialBackoffDelay(

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

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.

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

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...

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

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.

+    const sessions::ModelNeutralState& state) const {
+  // TODO(tim): Remove this, provide integration-test-only mechanism
+  // for override.
+  if (g_force_immediate_retry) {
+    return TimeDelta::FromSeconds(kInitialBackoffImmediateRetrySeconds);
+  }
+
+  if (IsError(state.last_get_key_result))
+    return TimeDelta::FromSeconds(kInitialBackoffRetrySeconds);
+  // Note: If we received a MIGRATION_DONE on download updates, then commit
+  // should not have taken place.  Moreover, if we receive a MIGRATION_DONE
+  // on commit, it means that download updates succeeded.  Therefore, we only
+  // need to check if either code is equal to SERVER_RETURN_MIGRATION_DONE,
+  // and not if there were any more serious errors requiring the long retry.
+  if (state.last_download_updates_result == SERVER_RETURN_MIGRATION_DONE ||
+      state.commit_result == SERVER_RETURN_MIGRATION_DONE) {

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

What's the thinking behind checking the commit's return value?  According to the
comment above (which I agree with) this will never matter.

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

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?

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

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.

+    return TimeDelta::FromSeconds(kInitialBackoffImmediateRetrySeconds);
+  }
+
+  return TimeDelta::FromSeconds(kInitialBackoffRetrySeconds);
+}
+
 void SyncSchedulerImpl::HandleContinuationError(
     const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ -907,7 +949,9 @@ void SyncSchedulerImpl::HandleContinuationError(
   }
 
   TimeDelta length = delay_provider_->GetDelay(
-      IsBackingOff() ? wait_interval_->length : TimeDelta::FromSeconds(1));
+      IsBackingOff() ? wait_interval_->length :
+          GetInitialBackoffDelay(
+              old_job.session->status_controller().model_neutral_state()));
 
   SDVLOG(2) << "In handle continuation error with "
             << SyncSessionJob::GetPurposeString(old_job.purpose)