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

sync/engine/sync_scheduler_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/bind.h"
@@ skipping 13 matching lines @@
 using base::TimeTicks;
 
 namespace syncer {
 
 using sessions::SyncSession;
 using sessions::SyncSessionSnapshot;
 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:
     case MIGRATION_DONE:
     case THROTTLED:
     case TRANSIENT_ERROR:
       return false;
     case NOT_MY_BIRTHDAY:
     case CLEAR_PENDING:
       // If we send terminate sync early then |sync_cycle_ended| notification
@@ skipping 846 matching lines @@
                     &SyncSchedulerImpl::PollTimerCallback);
 }
 
 void SyncSchedulerImpl::RestartWaiting() {
   CHECK(wait_interval_.get());
   wait_interval_->timer.Stop();
   wait_interval_->timer.Start(FROM_HERE, wait_interval_->length,
                               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_);
   if (DCHECK_IS_ON()) {
     if (IsBackingOff()) {
       DCHECK(wait_interval_->timer.IsRunning() || old_job.is_canary_job);
     }
   }
 
   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)
             << " job. The time delta(ms) is "
             << length.InMilliseconds();
 
   // This will reset the had_nudge variable as well.
   wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF,
                                         length));
   if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
@@ skipping 226 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer