Client-side changes to support retry GU.

Support GU retry command in sync engine. The command specifies a delay after which syncer should issue a GU to pick up updates missed by last GU.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244381

[haitaol1, 2014-01-03 19:52:23]

Send out for early feedbacks. It seems retry GU can leverage on existing code
for building GU message, error handling, exponential backoff, etc. Please
correct me in case I'm wrong and retry GU needs special support for those.

[rlarocque, 2014-01-03 21:17:15]

Thanks for sending this out early.

I've got a few suggestions.  Let me know if you'd like to discuss them in
person.

- I think that the retry GU looks more like a normal GU than it does a poll GU. 
It supports exponential backoff.  It can overlap with other sources.  For
example, it's not unthinkable that a single GU could be both a notification and
a retry nudge at the same time.  

This suggests to me that we should treat it more like the other causes of
"normal" sync cycles.  It should use the "GU_TRIGGER" origin value and set a
boolean value in the GetUpdatesTriggers message.  That way it would be able to
tell the server if a "retry GU" reason happens to overlap with a "notification"
or "nudge" or "refresh requested" GU.

- Timers can hide useful information.  Rather than tracking the state with a
boolean flag and a timer, I think it would be better to replace the boolean with
two timestamps: last_successful_gu and scheduled_retry_time.  We know that a
retry is required if last_successful_gu < retry_time.  This would make the code
that updates retry state a bit easier to read (we wouldn't need to explain why
we're setting or unsetting the boolean flag in various places).  It would also
make it possible to display pending retry status on about:sync and make it
easier to write unit tests for it.

We'd still need to keep the timer to trigger the retry when the timeout expires,
but at least we'd no longer be relying on it quite as much.

Finally, I'd also like to see the non-timer-related retry state pulled into the
NudgeTracker.  The NudgeTracker exists in part to make it easy to test the
SyncScheduler logic without having to deal with timers.

[haitaol, 2014-01-03 23:28:41]

Resolved per offline discussion. More inline.


On Fri, Jan 3, 2014 at 1:17 PM, <rlarocque@chromium.org> wrote:

> Thanks for sending this out early.
>
> I've got a few suggestions.  Let me know if you'd like to discuss them in
> person.
>
> - I think that the retry GU looks more like a normal GU than it does a
> poll GU.
> It supports exponential backoff.  It can overlap with other sources.  For
> example, it's not unthinkable that a single GU could be both a
> notification and
> a retry nudge at the same time.
>
> This suggests to me that we should treat it more like the other causes of
> "normal" sync cycles.  It should use the "GU_TRIGGER" origin value and set
> a
> boolean value in the GetUpdatesTriggers message.  That way it would be
> able to
> tell the server if a "retry GU" reason happens to overlap with a
> "notification"
> or "nudge" or "refresh requested" GU.
>

Decided to set a flag in non-retry GU to indicate whether retry GU is
collapsed into it. But
still use a dedicated GU source for retry GU.


>
> - Timers can hide useful information.  Rather than tracking the state with
> a
> boolean flag and a timer, I think it would be better to replace the
> boolean with
> two timestamps: last_successful_gu and scheduled_retry_time.  We know that
> a
> retry is required if last_successful_gu < retry_time.  This would make the
> code
> that updates retry state a bit easier to read (we wouldn't need to explain
> why
> we're setting or unsetting the boolean flag in various places).  It would
> also
> make it possible to display pending retry status on about:sync and make it
> easier to write unit tests for it.
>
> We'd still need to keep the timer to trigger the retry when the timeout
> expires,
> but at least we'd no longer be relying on it quite as much.
>
> Finally, I'd also like to see the non-timer-related retry state pulled
> into the
> NudgeTracker.  The NudgeTracker exists in part to make it easy to test the
> SyncScheduler logic without having to deal with timers.
>

Decided to use timestamps and move logic into nudgetracker.


>
> https://codereview.chromium.org/124083002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rlarocque, 2014-01-04 00:12:55]

Sounds good.  Did you forget to upload the latest patch?

[rlarocque, 2014-01-04 00:14:05]

+Pavel.

Pavel is working on the scheduler, too.  We should keep him in the loop since
his work could conflict with yours.

[haitaol1, 2014-01-06 22:14:02]

Updated as discussed. Also changed to apply same pre-sync check and post-sync
processing for three types of jobs: nudge, poll and retry. PTAL.

[rlarocque, 2014-01-06 23:00:33]

I disagree with the attempt to combine pre-sync and post-sync logic.  Doing so
will make it much harder to correctly implement some features we have planned
for the future.

As usual, let me know if you want to talk about it.

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:237:
get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
This is good, but I think this flag should also be set in the other case (where
origin == RETRY).

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:321: void BuildSupplementDownloadUpdatesImpl(
I realize this is a bit subjective, but I think this function should be split
into two.  The function only does two things, and the second of those two
depends on the value of a flag that you have to pass in explicitly.  If 50% of
the method depends on the value of a flag, I think it makes more sense to have
two different methods.

I think a similar comment could be made about the function on line 298, too. 
You could write that method better if it was split in two.  eg. DCHECK the
specific gu_origin rather than checking them both; logging the exact origin
rather than logging that it's one or the other.

That's a nit-level complaint, though.  You can leave it as is if you feel
strongly about it.

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:663: scoped_ptr<SyncSession>
session(SyncSession::Build(session_context_, this));
I'm strongly against this.  I spent a lot of time trying to eliminate the
SyncSession.  It's brittle and has caused lots of hard to detect crashes in the
past.

Looking to the future, I think an important goal will be to remove the concept
of the sync cycle.  Otherwise we'll never be able to properly handle
no-precommit-GU cycles.  The SyncSession is an obstacle that prevents this
change, and that's why I want to eliminate it.

This change would make it much harder to remove the SyncSession.  We should find
some other way to implement this.

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:710:
nudge_tracker_.RecordSuccessfulSyncCycle();
The SyncScheduler used to have code that looks like this.  I've tried to move it
in a different direction, and this would be a step backward.

At some point, we're going to have to let the NudgeTracker know when the syncer
has completed a GU, Commit, or both.  It will have to update it's state
differently in each case.  Calling "RecordSuccessfulSyncCycle()" may update the
status wrongly if this was a commit-only sync cycle.  There are also some
strange edge cases, like when a GU succeeds but the commit fails with a
transient error.

The syncer, about:sync and a few other components are still not quite ready for
that change.  However, when that change does come, we'll need to have failure
handling logic that is unique to each kind of job in the SyncScheduler.  This is
why I moved the error handling logic into separate functions and why I want to
keep it there.

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
next_retry_time_ < clock_->Now();
I'd strongly prefer to have the base::Time value passed in rather than have a
clock_ member.  Making the current time a function parameter makes testing
easier and allows us more flexibility in responding to timeouts.

[haitaol1, 2014-01-07 19:03:36]

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:237:
get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
origin == RETRY already indicates it's a retry GU, so seems redundant to set the
flag again.

On 2014/01/06 23:00:33, rlarocque wrote:
> This is good, but I think this flag should also be set in the other case
(where
> origin == RETRY).

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:321: void BuildSupplementDownloadUpdatesImpl(
On 2014/01/06 23:00:33, rlarocque wrote:
> I realize this is a bit subjective, but I think this function should be split
> into two.  The function only does two things, and the second of those two
> depends on the value of a flag that you have to pass in explicitly.  If 50% of
> the method depends on the value of a flag, I think it makes more sense to have
> two different methods.
> 
> I think a similar comment could be made about the function on line 298, too. 
> You could write that method better if it was split in two.  eg. DCHECK the
> specific gu_origin rather than checking them both; logging the exact origin
> rather than logging that it's one or the other.
> 
> That's a nit-level complaint, though.  You can leave it as is if you feel
> strongly about it.

Done.

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:663: scoped_ptr<SyncSession>
session(SyncSession::Build(session_context_, this));
On 2014/01/06 23:00:33, rlarocque wrote:
> I'm strongly against this.  I spent a lot of time trying to eliminate the
> SyncSession.  It's brittle and has caused lots of hard to detect crashes in
the
> past.
> 
> Looking to the future, I think an important goal will be to remove the concept
> of the sync cycle.  Otherwise we'll never be able to properly handle
> no-precommit-GU cycles.  The SyncSession is an obstacle that prevents this
> change, and that's why I want to eliminate it.
> 
> This change would make it much harder to remove the SyncSession.  We should
find
> some other way to implement this.

Done.

https://codereview.chromium.org/124083002/diff/190001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:710:
nudge_tracker_.RecordSuccessfulSyncCycle();
On 2014/01/06 23:00:33, rlarocque wrote:
> The SyncScheduler used to have code that looks like this.  I've tried to move
it
> in a different direction, and this would be a step backward.
> 
> At some point, we're going to have to let the NudgeTracker know when the
syncer
> has completed a GU, Commit, or both.  It will have to update it's state
> differently in each case.  Calling "RecordSuccessfulSyncCycle()" may update
the
> status wrongly if this was a commit-only sync cycle.  There are also some
> strange edge cases, like when a GU succeeds but the commit fails with a
> transient error.
> 
> The syncer, about:sync and a few other components are still not quite ready
for
> that change.  However, when that change does come, we'll need to have failure
> handling logic that is unique to each kind of job in the SyncScheduler.  This
is
> why I moved the error handling logic into separate functions and why I want to
> keep it there.

Done.

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
next_retry_time_ < clock_->Now();
I feel the other way. Using a clock member has the same effect in test and is
more consistent than passing clock every time. Also it makes the interface
cleaner.

On 2014/01/06 23:00:33, rlarocque wrote:
> I'd strongly prefer to have the base::Time value passed in rather than have a
> clock_ member.  Making the current time a function parameter makes testing
> easier and allows us more flexibility in responding to timeouts.

[rlarocque, 2014-01-07 19:43:40]

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:237:
get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
On 2014/01/07 19:03:37, haitaol1 wrote:
> origin == RETRY already indicates it's a retry GU, so seems redundant to set
the
> flag again.
> 
> On 2014/01/06 23:00:33, rlarocque wrote:
> > This is good, but I think this flag should also be set in the other case
> (where
> > origin == RETRY).
> 

This would make the logic to detect retry GUs harder on the server.  With the
current code, if the server wanted to know if a particular GU was a retry GU it
would have to check both the flag and the origin.  

I'd prefer to have the flag always set so that when this trickles down into the
logs, we could run queries that of the form (WHERE gu_msg.is_retry), rather than
(WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable to
expect people querying the logs to know they have to check one or the other. 
It's likely I'll have forgotten about this detail in a month or two.

(Yes, it would be possible to process these requests on the server side before
we write the logs to set the flag as appropriate, but I'd rather avoid it.  The
logging code is more likely to be correct if all it has to do is copy fields
directly from the request without doing any post-processing on them.)

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
next_retry_time_ < clock_->Now();
On 2014/01/07 19:03:37, haitaol1 wrote:
> I feel the other way. Using a clock member has the same effect in test and is
> more consistent than passing clock every time. Also it makes the interface
> cleaner.
> 
> On 2014/01/06 23:00:33, rlarocque wrote:
> > I'd strongly prefer to have the base::Time value passed in rather than have
a
> > clock_ member.  Making the current time a function parameter makes testing
> > easier and allows us more flexibility in responding to timeouts.
> 

It's not quite the same, though.

One trick we can use to make timer behavior very deterministic is to not query
the time when a timeout expires, but instead to assume the current time is
exactly when the timeout was scheduled for.

Consider three functions:

Example 1.

void onTimeout(base::Time scheduled_expiry) {
  if (nudge_tracker.IsRetryRequired(scheduled_expiry)) {
    ...
  }
}

Example 2.

void onTimeout() {
  if (nudge_tracker.IsRetryRequired(base::Time::Now())) {
    ...
  }
}

Example 3.

void onTimeout() {
  if (nudge_tracker.IsRetryRequired()) {
    ...
  }
}

The first and second forms are subtly different.  In the first case, we can bind
the expiry time to the timeout expiry function and guarantee that late timeouts
will not affect behavior in any way.  That can be a useful property, since it
prevents us from relying on timing assumptions.  

If we end up with more complicated scenarios, for example where one timeout
expiry results in a task that posts another timer for later, then this sort of
deterministic behavior could make it easier to test.  There are cases where it
would be hard to reset the mock clock's time part way through the test.

I'm currently using something similar to example 1 in
SyncSchedulerImpl::TypeUnthrottle().

It's probably not necessary to use the form from example 1 in your code. 
However, I think you should at least use the form from example 2, just to make
it easier to switch to something like example 1 in the future.

[haitaol1, 2014-01-07 23:34:48]

On 2014/01/07 19:43:40, rlarocque wrote:
> https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
> File sync/engine/download.cc (right):
> 
>
https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
> sync/engine/download.cc:237:
> get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
> On 2014/01/07 19:03:37, haitaol1 wrote:
> > origin == RETRY already indicates it's a retry GU, so seems redundant to set
> the
> > flag again.
> > 
> > On 2014/01/06 23:00:33, rlarocque wrote:
> > > This is good, but I think this flag should also be set in the other case
> > (where
> > > origin == RETRY).
> > 
> 
> This would make the logic to detect retry GUs harder on the server.  With the
> current code, if the server wanted to know if a particular GU was a retry GU
it
> would have to check both the flag and the origin.  
> 
> I'd prefer to have the flag always set so that when this trickles down into
the
> logs, we could run queries that of the form (WHERE gu_msg.is_retry), rather
than
> (WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable to
> expect people querying the logs to know they have to check one or the other. 
> It's likely I'll have forgotten about this detail in a month or two.
> 
> (Yes, it would be possible to process these requests on the server side before
> we write the logs to set the flag as appropriate, but I'd rather avoid it. 
The
> logging code is more likely to be correct if all it has to do is copy fields
> directly from the request without doing any post-processing on them.)
> 
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> File sync/sessions/nudge_tracker.cc (right):
> 
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
> next_retry_time_ < clock_->Now();
> On 2014/01/07 19:03:37, haitaol1 wrote:
> > I feel the other way. Using a clock member has the same effect in test and
is
> > more consistent than passing clock every time. Also it makes the interface
> > cleaner.
> > 
> > On 2014/01/06 23:00:33, rlarocque wrote:
> > > I'd strongly prefer to have the base::Time value passed in rather than
have
> a
> > > clock_ member.  Making the current time a function parameter makes testing
> > > easier and allows us more flexibility in responding to timeouts.
> > 
> 
> It's not quite the same, though.
> 
> One trick we can use to make timer behavior very deterministic is to not query
> the time when a timeout expires, but instead to assume the current time is
> exactly when the timeout was scheduled for.
> 
> Consider three functions:
> 
> Example 1.
> 
> void onTimeout(base::Time scheduled_expiry) {
>   if (nudge_tracker.IsRetryRequired(scheduled_expiry)) {
>     ...
>   }
> }
> 
> Example 2.
> 
> void onTimeout() {
>   if (nudge_tracker.IsRetryRequired(base::Time::Now())) {
>     ...
>   }
> }
> 
> Example 3.
> 
> void onTimeout() {
>   if (nudge_tracker.IsRetryRequired()) {
>     ...
>   }
> }
> 
> The first and second forms are subtly different.  In the first case, we can
bind
> the expiry time to the timeout expiry function and guarantee that late
timeouts
> will not affect behavior in any way.  That can be a useful property, since it
> prevents us from relying on timing assumptions.  
> 
> If we end up with more complicated scenarios, for example where one timeout
> expiry results in a task that posts another timer for later, then this sort of
> deterministic behavior could make it easier to test.  There are cases where it
> would be hard to reset the mock clock's time part way through the test.
> 
> I'm currently using something similar to example 1 in
> SyncSchedulerImpl::TypeUnthrottle().
> 
> It's probably not necessary to use the form from example 1 in your code. 
> However, I think you should at least use the form from example 2, just to make
> it easier to switch to something like example 1 in the future.

I'm afraid that I can't agree. Looking at SyncSchedulerImpl::TypeUnthrottle(),
I'm not sure what the benefit is to pass in now time instead of using a mock
clock. Actually I think it's incorrect to reuse previous now time to schedule
next TypeUnthrottle() because it doesn't take into account the time passed
between timer expiration and timer task execution, so the delay is not accurate.
And looks like sync_scheduler_unittest.cc only tests throttling types but not
unthrottling, so there's no real test benefit as I can see.

[rlarocque, 2014-01-08 00:45:17]

On 2014/01/07 23:34:48, haitaol1 wrote:
> On 2014/01/07 19:43:40, rlarocque wrote:
> >
https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
> > File sync/engine/download.cc (right):
> > 
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
> > sync/engine/download.cc:237:
> > get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
> > On 2014/01/07 19:03:37, haitaol1 wrote:
> > > origin == RETRY already indicates it's a retry GU, so seems redundant to
set
> > the
> > > flag again.
> > > 
> > > On 2014/01/06 23:00:33, rlarocque wrote:
> > > > This is good, but I think this flag should also be set in the other case
> > > (where
> > > > origin == RETRY).
> > > 
> > 
> > This would make the logic to detect retry GUs harder on the server.  With
the
> > current code, if the server wanted to know if a particular GU was a retry GU
> it
> > would have to check both the flag and the origin.  
> > 
> > I'd prefer to have the flag always set so that when this trickles down into
> the
> > logs, we could run queries that of the form (WHERE gu_msg.is_retry), rather
> than
> > (WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable to
> > expect people querying the logs to know they have to check one or the other.

> > It's likely I'll have forgotten about this detail in a month or two.
> > 
> > (Yes, it would be possible to process these requests on the server side
before
> > we write the logs to set the flag as appropriate, but I'd rather avoid it. 
> The
> > logging code is more likely to be correct if all it has to do is copy fields
> > directly from the request without doing any post-processing on them.)
> > 
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> > File sync/sessions/nudge_tracker.cc (right):
> > 
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> > sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
> > next_retry_time_ < clock_->Now();
> > On 2014/01/07 19:03:37, haitaol1 wrote:
> > > I feel the other way. Using a clock member has the same effect in test and
> is
> > > more consistent than passing clock every time. Also it makes the interface
> > > cleaner.
> > > 
> > > On 2014/01/06 23:00:33, rlarocque wrote:
> > > > I'd strongly prefer to have the base::Time value passed in rather than
> have
> > a
> > > > clock_ member.  Making the current time a function parameter makes
testing
> > > > easier and allows us more flexibility in responding to timeouts.
> > > 
> > 
> > It's not quite the same, though.
> > 
> > One trick we can use to make timer behavior very deterministic is to not
query
> > the time when a timeout expires, but instead to assume the current time is
> > exactly when the timeout was scheduled for.
> > 
> > Consider three functions:
> > 
> > Example 1.
> > 
> > void onTimeout(base::Time scheduled_expiry) {
> >   if (nudge_tracker.IsRetryRequired(scheduled_expiry)) {
> >     ...
> >   }
> > }
> > 
> > Example 2.
> > 
> > void onTimeout() {
> >   if (nudge_tracker.IsRetryRequired(base::Time::Now())) {
> >     ...
> >   }
> > }
> > 
> > Example 3.
> > 
> > void onTimeout() {
> >   if (nudge_tracker.IsRetryRequired()) {
> >     ...
> >   }
> > }
> > 
> > The first and second forms are subtly different.  In the first case, we can
> bind
> > the expiry time to the timeout expiry function and guarantee that late
> timeouts
> > will not affect behavior in any way.  That can be a useful property, since
it
> > prevents us from relying on timing assumptions.  
> > 
> > If we end up with more complicated scenarios, for example where one timeout
> > expiry results in a task that posts another timer for later, then this sort
of
> > deterministic behavior could make it easier to test.  There are cases where
it
> > would be hard to reset the mock clock's time part way through the test.
> > 
> > I'm currently using something similar to example 1 in
> > SyncSchedulerImpl::TypeUnthrottle().
> > 
> > It's probably not necessary to use the form from example 1 in your code. 
> > However, I think you should at least use the form from example 2, just to
make
> > it easier to switch to something like example 1 in the future.
> 
> I'm afraid that I can't agree. Looking at SyncSchedulerImpl::TypeUnthrottle(),
> I'm not sure what the benefit is to pass in now time instead of using a mock
> clock. Actually I think it's incorrect to reuse previous now time to schedule
> next TypeUnthrottle() because it doesn't take into account the time passed
> between timer expiration and timer task execution, so the delay is not
accurate.
> And looks like sync_scheduler_unittest.cc only tests throttling types but not
> unthrottling, so there's no real test benefit as I can see.

You're right about TypeUnthrottle().  It should use Time::Now() when it
reschedules its next timeout.

The way I see it, the benefits of explicitly passing the time in are:
- Explicit parameters are better than hidden parameters.  When we pass in the
time explicitly, we can guarantee that repeated calls to
NudgeTracker::IsSyncRequired(X) will always return the same result as long as X
is not modified and no one calls any non-const members of NudgeTracker.
- Having a MockClock makes tests more deterministic, but it does nothing to make
the real world more deterministic.
- A MockClock can get out of date with the rest of the test's state in a subtle
way.  If you set the MockClock's time at the beginning of a test then forget to
update it as the test scenario proceeds, that would be a non-obvious bug.  The
test might work correctly, but the scenario (eg. Running three sync cycles in 0
seconds) might be unrealistic from the NudgeTracker's point of view.

I suppose my fear is mostly around that last point.  If we use a MockClock, then
there's no incentive to force developers to write tests that expose the
NudgeTracker to a realistic passage of time.

[pavely, 2014-01-08 00:59:55]

https://codereview.chromium.org/124083002/diff/280001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/124083002/diff/280001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:700: return;
Please don't do early return from this function. There is a code at the end of
the function that needs to run regardless of sync cycle success.
There is a code to drop do_poll_after_credentials_updated_ flag and code  that
resets backoff timer in case we are in backoff and canary didn't run.

[haitaol1, 2014-01-08 19:06:00]

On 2014/01/08 00:45:17, rlarocque wrote:
> On 2014/01/07 23:34:48, haitaol1 wrote:
> > On 2014/01/07 19:43:40, rlarocque wrote:
> > >
> https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
> > > File sync/engine/download.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
> > > sync/engine/download.cc:237:
> > > get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
> > > On 2014/01/07 19:03:37, haitaol1 wrote:
> > > > origin == RETRY already indicates it's a retry GU, so seems redundant to
> set
> > > the
> > > > flag again.
> > > > 
> > > > On 2014/01/06 23:00:33, rlarocque wrote:
> > > > > This is good, but I think this flag should also be set in the other
case
> > > > (where
> > > > > origin == RETRY).
> > > > 
> > > 
> > > This would make the logic to detect retry GUs harder on the server.  With
> the
> > > current code, if the server wanted to know if a particular GU was a retry
GU
> > it
> > > would have to check both the flag and the origin.  
> > > 
> > > I'd prefer to have the flag always set so that when this trickles down
into
> > the
> > > logs, we could run queries that of the form (WHERE gu_msg.is_retry),
rather
> > than
> > > (WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable
to
> > > expect people querying the logs to know they have to check one or the
other.
> 
> > > It's likely I'll have forgotten about this detail in a month or two.
> > > 
> > > (Yes, it would be possible to process these requests on the server side
> before
> > > we write the logs to set the flag as appropriate, but I'd rather avoid it.

> > The
> > > logging code is more likely to be correct if all it has to do is copy
fields
> > > directly from the request without doing any post-processing on them.)
> > > 
> > >
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> > > File sync/sessions/nudge_tracker.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/124083002/diff/190001/sync/sessions/nudge_tra...
> > > sync/sessions/nudge_tracker.cc:61: return !next_retry_time_.is_null() &&
> > > next_retry_time_ < clock_->Now();
> > > On 2014/01/07 19:03:37, haitaol1 wrote:
> > > > I feel the other way. Using a clock member has the same effect in test
and
> > is
> > > > more consistent than passing clock every time. Also it makes the
interface
> > > > cleaner.
> > > > 
> > > > On 2014/01/06 23:00:33, rlarocque wrote:
> > > > > I'd strongly prefer to have the base::Time value passed in rather than
> > have
> > > a
> > > > > clock_ member.  Making the current time a function parameter makes
> testing
> > > > > easier and allows us more flexibility in responding to timeouts.
> > > > 
> > > 
> > > It's not quite the same, though.
> > > 
> > > One trick we can use to make timer behavior very deterministic is to not
> query
> > > the time when a timeout expires, but instead to assume the current time is
> > > exactly when the timeout was scheduled for.
> > > 
> > > Consider three functions:
> > > 
> > > Example 1.
> > > 
> > > void onTimeout(base::Time scheduled_expiry) {
> > >   if (nudge_tracker.IsRetryRequired(scheduled_expiry)) {
> > >     ...
> > >   }
> > > }
> > > 
> > > Example 2.
> > > 
> > > void onTimeout() {
> > >   if (nudge_tracker.IsRetryRequired(base::Time::Now())) {
> > >     ...
> > >   }
> > > }
> > > 
> > > Example 3.
> > > 
> > > void onTimeout() {
> > >   if (nudge_tracker.IsRetryRequired()) {
> > >     ...
> > >   }
> > > }
> > > 
> > > The first and second forms are subtly different.  In the first case, we
can
> > bind
> > > the expiry time to the timeout expiry function and guarantee that late
> > timeouts
> > > will not affect behavior in any way.  That can be a useful property, since
> it
> > > prevents us from relying on timing assumptions.  
> > > 
> > > If we end up with more complicated scenarios, for example where one
timeout
> > > expiry results in a task that posts another timer for later, then this
sort
> of
> > > deterministic behavior could make it easier to test.  There are cases
where
> it
> > > would be hard to reset the mock clock's time part way through the test.
> > > 
> > > I'm currently using something similar to example 1 in
> > > SyncSchedulerImpl::TypeUnthrottle().
> > > 
> > > It's probably not necessary to use the form from example 1 in your code. 
> > > However, I think you should at least use the form from example 2, just to
> make
> > > it easier to switch to something like example 1 in the future.
> > 
> > I'm afraid that I can't agree. Looking at
SyncSchedulerImpl::TypeUnthrottle(),
> > I'm not sure what the benefit is to pass in now time instead of using a mock
> > clock. Actually I think it's incorrect to reuse previous now time to
schedule
> > next TypeUnthrottle() because it doesn't take into account the time passed
> > between timer expiration and timer task execution, so the delay is not
> accurate.
> > And looks like sync_scheduler_unittest.cc only tests throttling types but
not
> > unthrottling, so there's no real test benefit as I can see.
> 
> You're right about TypeUnthrottle().  It should use Time::Now() when it
> reschedules its next timeout.
> 
> The way I see it, the benefits of explicitly passing the time in are:
> - Explicit parameters are better than hidden parameters.  When we pass in the
> time explicitly, we can guarantee that repeated calls to
> NudgeTracker::IsSyncRequired(X) will always return the same result as long as
X
> is not modified and no one calls any non-const members of NudgeTracker.
> - Having a MockClock makes tests more deterministic, but it does nothing to
make
> the real world more deterministic.
> - A MockClock can get out of date with the rest of the test's state in a
subtle
> way.  If you set the MockClock's time at the beginning of a test then forget
to
> update it as the test scenario proceeds, that would be a non-obvious bug.  The
> test might work correctly, but the scenario (eg. Running three sync cycles in
0
> seconds) might be unrealistic from the NudgeTracker's point of view.
> 
> I suppose my fear is mostly around that last point.  If we use a MockClock,
then
> there's no incentive to force developers to write tests that expose the
> NudgeTracker to a realistic passage of time.

By realistic passage of time, do you mean test with real clock. That's certainly
an option and 
I think some scheduler tests (e.g. ThrottlingExpiresFromNudge) use real time
already. But I don't see
how passing now time is realistic. Actually I think it created an unrealistic
context
and could lead to some subtle bugs, e.g. the unthrottle will get stuck if the
passed now time is not updated,
or unthrottle prematurely if timer timeout is set incorrectly.

[haitaol1, 2014-01-08 19:06:37]

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/124083002/diff/190001/sync/engine/download.cc...
sync/engine/download.cc:237:
get_updates->set_is_retry(nudge_tracker.IsRetryRequired());
On 2014/01/07 19:43:41, rlarocque wrote:
> On 2014/01/07 19:03:37, haitaol1 wrote:
> > origin == RETRY already indicates it's a retry GU, so seems redundant to set
> the
> > flag again.
> > 
> > On 2014/01/06 23:00:33, rlarocque wrote:
> > > This is good, but I think this flag should also be set in the other case
> > (where
> > > origin == RETRY).
> > 
> 
> This would make the logic to detect retry GUs harder on the server.  With the
> current code, if the server wanted to know if a particular GU was a retry GU
it
> would have to check both the flag and the origin.  
> 
> I'd prefer to have the flag always set so that when this trickles down into
the
> logs, we could run queries that of the form (WHERE gu_msg.is_retry), rather
than
> (WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable to
> expect people querying the logs to know they have to check one or the other. 
> It's likely I'll have forgotten about this detail in a month or two.
> 
> (Yes, it would be possible to process these requests on the server side before
> we write the logs to set the flag as appropriate, but I'd rather avoid it. 
The
> logging code is more likely to be correct if all it has to do is copy fields
> directly from the request without doing any post-processing on them.)

Done.

https://codereview.chromium.org/124083002/diff/280001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/124083002/diff/280001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_impl.cc:700: return;
On 2014/01/08 00:59:56, pavely wrote:
> Please don't do early return from this function. There is a code at the end of
> the function that needs to run regardless of sync cycle success.
> There is a code to drop do_poll_after_credentials_updated_ flag and code  that
> resets backoff timer in case we are in backoff and canary didn't run.

Done.

[haitaol1, 2014-01-08 19:58:16]

Patch refreshed. PTAL

[rlarocque, 2014-01-08 20:30:07]

I have some comments about testing, but most of the patch looks good.

https://codereview.chromium.org/124083002/diff/430001/sync/engine/download.h
File sync/engine/download.h (right):

https://codereview.chromium.org/124083002/diff/430001/sync/engine/download.h#...
sync/engine/download.h:84: SYNC_EXPORT_PRIVATE void
BuildDownloadUpdatesForRetryImpl(
The reason we have separate Impl methods is to allow us to test the building of
request messages.  You should probably add a test to download_unittest.cc as you
add this function, and maybe update a few of the existing tests too.  (eg. Test
that the is_retry flag is set properly.)

Also, nit: please separate these two functions with a newline and add a comment
to the second one.

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:37: bool NudgeTracker::IsSyncRequired() const {
I'm pretty sure this should return true if IsRetryRequired() is true.  I think
it's currently assumed that IsGetUpdatesRequired() necessarily implies
IsSyncRequired().  

This may change when we split we eventually define a separate
IsCommitRequired(), but we should probably keep the assumption for now.

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.h (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.h:14: #include "base/time/clock.h"
I think you can remove this now.

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker_unittest.cc:740: 
Since you went through all this trouble to add support for retries to the nudge
tracker and make it testable, you should probably add some tests for it here.

[haitaol1, 2014-01-09 00:30:43]

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:37: bool NudgeTracker::IsSyncRequired() const {
I didn't call IsRetryRequired() here because that will execute the normal sync
path. But I'd like to avoid mixing for now to keep it simple given that retry
and pre-commit GU will co-exist for a while. Also IsGetUpdatesRequired() checks
invalidation but IsSyncRequired() 
doesn't. Not sure if that's intentional.

On 2014/01/08 20:30:08, rlarocque wrote:
> I'm pretty sure this should return true if IsRetryRequired() is true.  I think
> it's currently assumed that IsGetUpdatesRequired() necessarily implies
> IsSyncRequired().  
> 
> This may change when we split we eventually define a separate
> IsCommitRequired(), but we should probably keep the assumption for now.

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.h (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.h:14: #include "base/time/clock.h"
On 2014/01/08 20:30:08, rlarocque wrote:
> I think you can remove this now.

Done.

[haitaol1, 2014-01-09 00:33:37]

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker_unittest.cc:740: 
working on tests

On 2014/01/08 20:30:08, rlarocque wrote:
> Since you went through all this trouble to add support for retries to the
nudge
> tracker and make it testable, you should probably add some tests for it here.

[rlarocque, 2014-01-09 00:37:20]

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/124083002/diff/430001/sync/sessions/nudge_tra...
sync/sessions/nudge_tracker.cc:37: bool NudgeTracker::IsSyncRequired() const {
On 2014/01/09 00:30:44, haitaol1 wrote:
> I didn't call IsRetryRequired() here because that will execute the normal sync
> path. But I'd like to avoid mixing for now to keep it simple given that retry
> and pre-commit GU will co-exist for a while. Also IsGetUpdatesRequired()
checks
> invalidation but IsSyncRequired() 
> doesn't. Not sure if that's intentional.

At the moment, I'm not sure either.  :)

It's been a while.  Maybe there was some intent to start making the two diverge.

Anyway, if we're already at the point where IsGetUpdatesRequired() does not
necessarily imply that IsSyncRequired(), then I see no problem in taking another
step in that direction.  You can keep the code as is.

[haitaol1, 2014-01-10 00:15:58]

Tests added. PTAL.

https://codereview.chromium.org/124083002/diff/430001/sync/engine/download.h
File sync/engine/download.h (right):

https://codereview.chromium.org/124083002/diff/430001/sync/engine/download.h#...
sync/engine/download.h:84: SYNC_EXPORT_PRIVATE void
BuildDownloadUpdatesForRetryImpl(
On 2014/01/08 20:30:08, rlarocque wrote:
> The reason we have separate Impl methods is to allow us to test the building
of
> request messages.  You should probably add a test to download_unittest.cc as
you
> add this function, and maybe update a few of the existing tests too.  (eg.
Test
> that the is_retry flag is set properly.)
> 
> Also, nit: please separate these two functions with a newline and add a
comment
> to the second one.

Done.

[rlarocque, 2014-01-10 00:42:00]

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1309:
scheduler()->OnReceivedGuRetryDelaySeconds(1);
Hmm... These tests will be slow.  We went out of our way to use a
MockDelayProvider to make these tests faster.  It would be nice to avoid
regressions.

Here's an alternative.  You could change the signature of
OnReceivedGuRetryDelaySeconds(int) to OnReceivedGuRetryDelay(base::TimeDelta). 
The new signature would be better that the original because it is more
expressive.  It will also let you pass in a really short timeout (ie. 1ms) for
tests here.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1324: TEST_F(SyncSchedulerTest,
FaileRetry) {
typo: FailRetry?

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1370: EXPECT_LT(times[0] - t1,
base::TimeDelta::FromSeconds(1));
This looks flaky.  What if the timer takes an unusually long time to fire?

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1380: EXPECT_NEAR((times[1] -
times[0]).InSeconds(), 2, 0.2);
This one is almost certain to be flaky.  We can't have tests that rely on
accurate timers.  Every time we've tried it, we ended up having to disable the
tests as flaky then rewrite them.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/syncer_prot...
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/124083002/diff/550001/sync/engine/syncer_prot...
sync/engine/syncer_proto_util.cc:437: 
nit: remove extra space.

[haitaol1, 2014-01-10 22:10:34]

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1309:
scheduler()->OnReceivedGuRetryDelaySeconds(1);
On 2014/01/10 00:42:00, rlarocque wrote:
> Hmm... These tests will be slow.  We went out of our way to use a
> MockDelayProvider to make these tests faster.  It would be nice to avoid
> regressions.
> 
> Here's an alternative.  You could change the signature of
> OnReceivedGuRetryDelaySeconds(int) to OnReceivedGuRetryDelay(base::TimeDelta).

> The new signature would be better that the original because it is more
> expressive.  It will also let you pass in a really short timeout (ie. 1ms) for
> tests here.

Done.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1324: TEST_F(SyncSchedulerTest,
FaileRetry) {
On 2014/01/10 00:42:00, rlarocque wrote:
> typo: FailRetry?

Done.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1370: EXPECT_LT(times[0] - t1,
base::TimeDelta::FromSeconds(1));
On 2014/01/10 00:42:00, rlarocque wrote:
> This looks flaky.  What if the timer takes an unusually long time to fire?

Done.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1380: EXPECT_NEAR((times[1] -
times[0]).InSeconds(), 2, 0.2);
On 2014/01/10 00:42:00, rlarocque wrote:
> This one is almost certain to be flaky.  We can't have tests that rely on
> accurate timers.  Every time we've tried it, we ended up having to disable the
> tests as flaky then rewrite them.

Done.

https://codereview.chromium.org/124083002/diff/550001/sync/engine/syncer_prot...
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/124083002/diff/550001/sync/engine/syncer_prot...
sync/engine/syncer_proto_util.cc:437: 
On 2014/01/10 00:42:00, rlarocque wrote:
> nit: remove extra space.

Done.

[rlarocque, 2014-01-10 22:41:12]

Much better.  I've got a few nits, but otherwise LGTM.

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:223: return
scheduler_->retry_timer_.GetCurrentDelay();
This seems like a grey area in the API.  I'm not sure that it's safe to call
GetCurrentDelay() on the timer if the timer has already fired.  Though looking
at the implementation, I see no reason why it wouldn't work.

Since these checks are not really essential to your tests, I guess this is OK. 
You could always remove them if this breaks.

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1376:
WithoutArgs(VerifyRetryTimerDelay(this, delay1)),
nit: Maybe put this into an expectation just before the sync cycle is allowed to
run (ie. just before line 1382).

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1378:
WithoutArgs(VerifyRetryTimerDelay(this, delay2)),
nit: I think it would be more clear to perform this second timer delay
verification a few lines below.  (ie. just after line 1382).

[haitaol1, 2014-01-10 23:45:02]

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1376:
WithoutArgs(VerifyRetryTimerDelay(this, delay1)),
On 2014/01/10 22:41:13, rlarocque wrote:
> nit: Maybe put this into an expectation just before the sync cycle is allowed
to
> run (ie. just before line 1382).

I'd like to verify pre/post conditions tightly around NormalSyncShare to avoid
interference of other code.

[rlarocque, 2014-01-10 23:47:58]

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/124083002/diff/860001/sync/engine/sync_schedu...
sync/engine/sync_scheduler_unittest.cc:1376:
WithoutArgs(VerifyRetryTimerDelay(this, delay1)),
On 2014/01/10 23:45:03, haitaol1 wrote:
> On 2014/01/10 22:41:13, rlarocque wrote:
> > nit: Maybe put this into an expectation just before the sync cycle is
allowed
> to
> > run (ie. just before line 1382).
> 
> I'd like to verify pre/post conditions tightly around NormalSyncShare to avoid
> interference of other code.

That's valid.  I suggested alternatives because:
- I dislike GMock, and would prefer to rely on it as little as possible.
- Seeing the retry timer get verified against two different values within two
lines of each other looks a bit suspicious.

Neither argument is very strong.  You can leave it as is if you feel strongly
about it.

[commit-bot: I haz the power, 2014-01-11 00:20:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/124083002/1060001

[commit-bot: I haz the power, 2014-01-11 23:00:44]

Change committed as 244381