sync: GU retry with less explicit TimeTicks logic

sync/sessions/nudge_tracker.cc

Index: sync/sessions/nudge_tracker.cc
diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index f96d3464826e86653fb04d515c7d9575d891f2be..0681c84adfe59b15caad4e686065d961e412d30a 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -44,11 +44,11 @@ bool NudgeTracker::IsSyncRequired() const {
   return false;
 }
 
-bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const {
+bool NudgeTracker::IsGetUpdatesRequired() const {
   if (invalidations_out_of_sync_)
     return true;
 
-  if (IsRetryRequired(now))
+  if (IsRetryRequired())
     return true;
 
   for (TypeTrackerMap::const_iterator it = type_trackers_.begin();
@@ -60,16 +60,22 @@ bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const {
   return false;
 }
 
-bool NudgeTracker::IsRetryRequired(base::TimeTicks now) const {
-  return !next_retry_time_.is_null() && next_retry_time_ < now;
+bool NudgeTracker::IsRetryRequired() const {
+  if (sync_cycle_start_time_.is_null())
+    return false;
+
+  if (current_retry_time_.is_null())
+    return false;
+
+  return current_retry_time_ < sync_cycle_start_time_;
 }
 
-void NudgeTracker::RecordSuccessfulSyncCycle(base::TimeTicks now) {
+void NudgeTracker::RecordSuccessfulSyncCycle() {
   updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN;
-  last_successful_sync_time_ = now;
 
-  if (next_retry_time_ < now)
-    next_retry_time_ = base::TimeTicks();
+  // If a retry was required, we've just serviced it.  Unset the flag.
+  if (IsRetryRequired())
+    current_retry_time_ = base::TimeTicks();
 
   // A successful cycle while invalidations are enabled puts us back into sync.
   invalidations_out_of_sync_ = !invalidations_enabled_;
@@ -239,6 +245,19 @@ void NudgeTracker::FillProtoMessage(
   type_trackers_.find(type)->second.FillGetUpdatesTriggersMessage(msg);
 }
 
+void NudgeTracker::SetSyncCycleStartTime(base::TimeTicks now) {
+  sync_cycle_start_time_ = now;
+
+  // We store updates to our GU retry timer in next_retry_time_.  The one
+  // currently in effect is stored in current_retry_time_.  Here is where we
+  // apply the pending update from next_retry_time_ to current_retry_time_.  The
+  // current_retry_time_ will be overwritten in the process.
+  if (!next_retry_time_.is_null()) {

[haitaol1, 2014/01/28 23:07:23]

Probably don't set current_retry_time if it's not null, which means last retry
GU failed. So current_retry_time always stores the time of earliest retry that
hasn't succeeded.

[rlarocque, 2014/01/28 23:17:09]

Correct.  That was my intention.  At least, that's the behavior that makes the
most sense to me.

If a retry GU fails, then we'll go into exponential backoff.  It would make
sense to me that IsRetryRequired() will be true when that backoff expires and we
make another attempt at the GU retry.

Leaving current_retry_time_ alone in the failure case seems to me a not-too-bad
way of achieving that goal.

Am I missing some obvious side-effects of this choice?  Or is my understanding
of the desired behavior wrong?

+    current_retry_time_ = next_retry_time_;
+    next_retry_time_ = base::TimeTicks();
+  }
+}
+
 void NudgeTracker::SetHintBufferSize(size_t size) {
   for (TypeTrackerMap::iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
@@ -246,5 +265,9 @@ void NudgeTracker::SetHintBufferSize(size_t size) {
   }
 }
 
+void NudgeTracker::SetNextRetryTime(base::TimeTicks retry_time) {
+  next_retry_time_ = retry_time;
+}
+
 }  // namespace sessions
 }  // namespace syncer