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

components/sync/engine_impl/cycle/data_type_tracker.cc

Index: components/sync/engine_impl/cycle/data_type_tracker.cc
diff --git a/components/sync/engine_impl/cycle/data_type_tracker.cc b/components/sync/engine_impl/cycle/data_type_tracker.cc
index 732bcdc172defb5868e7119ef21562d1018b11f0..e76f820cb19b7830bd26e55d9741e3e64090fa37 100644
--- a/components/sync/engine_impl/cycle/data_type_tracker.cc
+++ b/components/sync/engine_impl/cycle/data_type_tracker.cc
@@ -7,10 +7,37 @@
 #include <algorithm>
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "components/sync/engine_impl/cycle/nudge_tracker.h"
 
 namespace syncer {
 
+namespace {
+
+#define ENUM_CASE(x) \
+  case x:            \
+    return #x;       \
+    break;
+
+}  // namespace
+
+WaitInterval::WaitInterval() : mode(UNKNOWN) {}
+
+WaitInterval::WaitInterval(BlockingMode mode, base::TimeDelta length)
+    : mode(mode), length(length) {}
+
+WaitInterval::~WaitInterval() {}
+
+const char* WaitInterval::GetModeString(BlockingMode mode) {
+  switch (mode) {
+    ENUM_CASE(UNKNOWN);
+    ENUM_CASE(EXPONENTIAL_BACKOFF);
+    ENUM_CASE(THROTTLED);
+  }
+  NOTREACHED();
+  return "";
+}
+
 DataTypeTracker::DataTypeTracker()
     : local_nudge_count_(0),
       local_refresh_request_count_(0),
@@ -99,7 +126,7 @@ void DataTypeTracker::RecordCommitConflict() {
 void DataTypeTracker::RecordSuccessfulSyncCycle() {
   // If we were throttled, then we would have been excluded from this cycle's
   // GetUpdates and Commit actions.  Our state remains unchanged.
-  if (IsThrottled())
+  if (IsThrottled() || IsBackedOff())
     return;
 
   local_nudge_count_ = 0;
@@ -131,11 +158,12 @@ void DataTypeTracker::UpdatePayloadBufferSize(size_t new_size) {
 }
 
 bool DataTypeTracker::IsSyncRequired() const {
-  return !IsThrottled() && (HasLocalChangePending() || IsGetUpdatesRequired());
+  return !IsThrottled() && !IsBackedOff() &&
+         (HasLocalChangePending() || IsGetUpdatesRequired());
 }
 
 bool DataTypeTracker::IsGetUpdatesRequired() const {
-  return !IsThrottled() &&
+  return !IsThrottled() && !IsBackedOff() &&
          (HasRefreshRequestPending() || HasPendingInvalidation() ||
           IsInitialSyncRequired() || IsSyncRequiredToResolveConflict());
 }
@@ -164,6 +192,8 @@ void DataTypeTracker::SetLegacyNotificationHint(
     sync_pb::DataTypeProgressMarker* progress) const {
   DCHECK(!IsThrottled())
       << "We should not make requests if the type is throttled.";
+  DCHECK(!IsBackedOff())
+      << "We should not make requests if the type is backed off.";
 
   if (!pending_invalidations_.empty() &&
       !pending_invalidations_.back()->IsUnknownVersion()) {
@@ -204,7 +234,13 @@ void DataTypeTracker::FillGetUpdatesTriggersMessage(
 }
 
 bool DataTypeTracker::IsThrottled() const {
-  return !unthrottle_time_.is_null();
+  return wait_interval_.get() &&
+         wait_interval_->mode == WaitInterval::THROTTLED;
+}
+
+bool DataTypeTracker::IsBackedOff() const {
+  return wait_interval_.get() &&
+         wait_interval_->mode == WaitInterval::EXPONENTIAL_BACKOFF;
 }
 
 base::TimeDelta DataTypeTracker::GetTimeUntilUnthrottle(
@@ -213,17 +249,44 @@ base::TimeDelta DataTypeTracker::GetTimeUntilUnthrottle(
     NOTREACHED();
     return base::TimeDelta::FromSeconds(0);
   }
-  return std::max(base::TimeDelta::FromSeconds(0), unthrottle_time_ - now);
+  return std::max(base::TimeDelta::FromSeconds(0), unblock_time_ - now);
+}
+
+base::TimeDelta DataTypeTracker::GetTimeUntilUnbackoff(
+    base::TimeTicks now) const {
+  if (!IsBackedOff()) {

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

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.

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

Done.

+    NOTREACHED();
+    return base::TimeDelta::FromSeconds(0);
+  }
+  return std::max(base::TimeDelta::FromSeconds(0), unblock_time_ - now);
+}
+
+base::TimeDelta DataTypeTracker::GetLastUnbackoffInterval() const {
+  if (!IsBackedOff()) {
+    NOTREACHED();
+    return base::TimeDelta::FromSeconds(0);
+  }
+  return wait_interval_->length;
 }
 
 void DataTypeTracker::ThrottleType(base::TimeDelta duration,
                                    base::TimeTicks now) {
-  unthrottle_time_ = std::max(unthrottle_time_, now + duration);
+  unblock_time_ = std::max(unblock_time_, now + duration);
+  wait_interval_ =
+      base::MakeUnique<WaitInterval>(WaitInterval::THROTTLED, duration);
+}
+
+void DataTypeTracker::BackoffType(base::TimeDelta duration,
+                                  base::TimeTicks now) {
+  unblock_time_ = std::max(unblock_time_, now + duration);
+  wait_interval_ = base::MakeUnique<WaitInterval>(
+      WaitInterval::EXPONENTIAL_BACKOFF, duration);
 }
 
-void DataTypeTracker::UpdateThrottleState(base::TimeTicks now) {
-  if (now >= unthrottle_time_) {
-    unthrottle_time_ = base::TimeTicks();
+void DataTypeTracker::UpdateThrottleOrBackoffState(base::TimeTicks now) {
+  if (now >= unblock_time_) {

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

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())?

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

Done.

+    unblock_time_ = base::TimeTicks();
+    wait_interval_.reset();
   }
 }