Delay autofill commits to reduce client to server traffic.

We introduce the concept of nudge delay and each datatype is assigned a delay from one of the enums. 

Autofill is assigned the value PIGGYBACK_WITH_ANOTHER_CHANGE.

Our sceheduler did not handle the case of 2 nudges being schedule with the first one being scheduled for a later time than the second one. Fixed that and added unit tests to it.
BUG=106250
TEST=sync_unit_tests.exe, unit_tests.exe, integration_tests.exe


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114941

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115072

[lipalani1, 2011-12-03 01:10:09]

Please review.

[tim (not reviewing), 2011-12-06 20:15:57]

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:552:
pending_nudge_->scheduled_start);
This was explicitly not the goal of the implementation. Why do we need this now?
E.g. for sessions, we have a delay of 11 seconds to coalesce. We actually only
want to sync once things have quiesced for 11 seconds, and this would break
that.

In either case, shouldn't this be a separate change?

[lipalani1, 2011-12-06 20:44:06]

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:552:
pending_nudge_->scheduled_start);
This is a needed change and is deliberate. 
Need: When you put an autofill change to sync 2 hours later and then a bookmark
change comes along we want to sync right now rather than waiting for 2 hours.

Problem with existing impl: Since we keep pushing the new syncs to later time
there is a possibility that the user's browser does not sync for extended period
of time. If the user keeps opening new URLs or whatever continuously we would
not be syncing ANY change for that whole time period and in general this could
lead to bugs that say "my sync is not working". The new impl introduces
predictability which is important at this layer.

Compromise: I do believe the new impl is the way to go :) However I had a backup
plan cooked up in case this is not acceptable. For autofill type of changes we
need not schedule a sync at all. We can let the long poll interval take care of
it. Ideally some other change would occur and sync autofill. If the user has not
made any other changes it is probably OK to wait until the long poll interval.
But we should hash out why we should/should not use the new logic first.
On 2011/12/06 20:15:57, timsteele wrote:
> This was explicitly not the goal of the implementation. Why do we need this
now?
> E.g. for sessions, we have a delay of 11 seconds to coalesce. We actually only
> want to sync once things have quiesced for 11 seconds, and this would break
> that.
> 
> In either case, shouldn't this be a separate change?

[tim (not reviewing), 2011-12-06 20:58:45]

Actually I'm a bit confused now.  The "freshness condition" check was there
so that nudges that came in and forced an earlier sync than the one
originally planned would run but the older scheduled job would not.  I
recall a discussion around sessions and wanting this timer reset behavior
which is what sparked my earlier comment, but looking at the code I
remember adding this check in the initial SyncerThread2 impl, because the
old impl would just wake up from it's loop when a new nudge came in.  It
seems like either your change here depends on the same freshness check, or
at some point the behavior regressed and that check became obsolete
(possibly because we now always store the pending_nudge_ variable?).


On Tue, Dec 6, 2011 at 12:44 PM, <lipalani@chromium.org> wrote:

>
> http://codereview.chromium.**org/8787006/diff/1006/chrome/**
>
browser/sync/engine/sync_**scheduler.cc<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc>
> File chrome/browser/sync/engine/**sync_scheduler.cc (right):
>
> http://codereview.chromium.**org/8787006/diff/1006/chrome/**
>
browser/sync/engine/sync_**scheduler.cc#newcode552<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc#newcode552>
> chrome/browser/sync/engine/**sync_scheduler.cc:552:
> pending_nudge_->scheduled_**start);
> This is a needed change and is deliberate.
> Need: When you put an autofill change to sync 2 hours later and then a
> bookmark change comes along we want to sync right now rather than
> waiting for 2 hours.
>
> Problem with existing impl: Since we keep pushing the new syncs to later
> time there is a possibility that the user's browser does not sync for
> extended period of time. If the user keeps opening new URLs or whatever
> continuously we would not be syncing ANY change for that whole time
> period and in general this could lead to bugs that say "my sync is not
> working". The new impl introduces predictability which is important at
> this layer.
>
> Compromise: I do believe the new impl is the way to go :) However I had
> a backup plan cooked up in case this is not acceptable. For autofill
> type of changes we need not schedule a sync at all. We can let the long
> poll interval take care of it. Ideally some other change would occur and
> sync autofill. If the user has not made any other changes it is probably
> OK to wait until the long poll interval. But we should hash out why we
> should/should not use the new logic first.
>
> On 2011/12/06 20:15:57, timsteele wrote:
>
>> This was explicitly not the goal of the implementation. Why do we need
>>
> this now?
>
>> E.g. for sessions, we have a delay of 11 seconds to coalesce. We
>>
> actually only
>
>> want to sync once things have quiesced for 11 seconds, and this would
>>
> break
>
>> that.
>>
>
>  In either case, shouldn't this be a separate change?
>>
>
>
http://codereview.chromium.**org/8787006/<http://codereview.chromium.org/8787...
>

[lipalani1, 2011-12-06 22:37:08]

hmm not sure I understand. Let me explain the current state and what this impl
does:
Current state:
Let us assume there are no errors.
A nudge comes in with Delay x(greater than 0) so we store it.
A new nudge come for y(Does not matter if x < y or x > y) we will coalesce x
with y and then drop it. then execute the nudge at x.

So in effect we execute the first nudge that was scheduled regardless of the
delay that was set to. Any nudges that come before the execution of this nudge
is simply dropped.
(A little bit impl detail: In DoSyncSessionJob which is the task posted to
execute, we compare the session pointer of the posted job parameter with the
pending_nudge's sessions. They would be equal as pending nudge was never reset)

This is slightly confusing and could lead to a case where sync is stalled for a
long time(if we scheduled a nudge 2 hours from now).
New behavior.
Assume no errors.
1. A nudge comes in with delay x and we store it.
2. A new nudge comes in with delay y. We will coalesce it. Then we will post a
nudge at the earliest time of x and y and drop the other nudge. Before that We
will destroy the pending nudge and recreate it with the new data. So when the
posted job for x comes along it will not execute as the session pointer would be
different. 

This is a cleaner implementation. Let me add some more comments and post another
patch(I also found a small bug in nudge posting which is left as a todo in the
code, I will fix that as well). But in the mean time we can discuss if you agree
with this architecture.
On 2011/12/06 20:58:45, timsteele wrote:
> Actually I'm a bit confused now.  The "freshness condition" check was there
> so that nudges that came in and forced an earlier sync than the one
> originally planned would run but the older scheduled job would not.  I
> recall a discussion around sessions and wanting this timer reset behavior
> which is what sparked my earlier comment, but looking at the code I
> remember adding this check in the initial SyncerThread2 impl, because the
> old impl would just wake up from it's loop when a new nudge came in.  It
> seems like either your change here depends on the same freshness check, or
> at some point the behavior regressed and that check became obsolete
> (possibly because we now always store the pending_nudge_ variable?).
> 
> 
> On Tue, Dec 6, 2011 at 12:44 PM, <mailto:lipalani@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/8787006/diff/1006/chrome/**
> >
>
browser/sync/engine/sync_**scheduler.cc<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc>
> > File chrome/browser/sync/engine/**sync_scheduler.cc (right):
> >
> > http://codereview.chromium.**org/8787006/diff/1006/chrome/**
> >
>
browser/sync/engine/sync_**scheduler.cc#newcode552<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc#newcode552>
> > chrome/browser/sync/engine/**sync_scheduler.cc:552:
> > pending_nudge_->scheduled_**start);
> > This is a needed change and is deliberate.
> > Need: When you put an autofill change to sync 2 hours later and then a
> > bookmark change comes along we want to sync right now rather than
> > waiting for 2 hours.
> >
> > Problem with existing impl: Since we keep pushing the new syncs to later
> > time there is a possibility that the user's browser does not sync for
> > extended period of time. If the user keeps opening new URLs or whatever
> > continuously we would not be syncing ANY change for that whole time
> > period and in general this could lead to bugs that say "my sync is not
> > working". The new impl introduces predictability which is important at
> > this layer.
> >
> > Compromise: I do believe the new impl is the way to go :) However I had
> > a backup plan cooked up in case this is not acceptable. For autofill
> > type of changes we need not schedule a sync at all. We can let the long
> > poll interval take care of it. Ideally some other change would occur and
> > sync autofill. If the user has not made any other changes it is probably
> > OK to wait until the long poll interval. But we should hash out why we
> > should/should not use the new logic first.
> >
> > On 2011/12/06 20:15:57, timsteele wrote:
> >
> >> This was explicitly not the goal of the implementation. Why do we need
> >>
> > this now?
> >
> >> E.g. for sessions, we have a delay of 11 seconds to coalesce. We
> >>
> > actually only
> >
> >> want to sync once things have quiesced for 11 seconds, and this would
> >>
> > break
> >
> >> that.
> >>
> >
> >  In either case, shouldn't this be a separate change?
> >>
> >
> >
>
http://codereview.chromium.**org/8787006/%3Chttp://codereview.chromium.org/87...>
> >

[tim (not reviewing), 2011-12-06 23:15:56]

What I'm saying is the freshness condition (search sync_scheduler for that
text) was there to handle a case where a nudge comes in with greater
(without loss of generality) delay, we would sync, and then not want to run
the later nudge because we just ran a sync that "took care" of what both
nudges wanted to do.  So the fact that we don't handle this at all now is a
regression. Fixing it is great, but you should understand what the
freshness-check was there for and remove it if it is no longer needed.

On Tue, Dec 6, 2011 at 2:37 PM, <lipalani@chromium.org> wrote:

> hmm not sure I understand. Let me explain the current state and what this
> impl
> does:
> Current state:
> Let us assume there are no errors.
> A nudge comes in with Delay x(greater than 0) so we store it.
> A new nudge come for y(Does not matter if x < y or x > y) we will coalesce
> x
> with y and then drop it. then execute the nudge at x.
>
> So in effect we execute the first nudge that was scheduled regardless of
> the
> delay that was set to. Any nudges that come before the execution of this
> nudge
> is simply dropped.
>

It is not simply dropped.  The sessions are coalesced.

> (A little bit impl detail: In DoSyncSessionJob which is the task posted to
> execute, we compare the session pointer of the posted job parameter with
> the
> pending_nudge's sessions. They would be equal as pending nudge was never
> reset)
>
> This is slightly confusing and could lead to a case where sync is stalled
> for a
> long time(if we scheduled a nudge 2 hours from now).
>

The existing behavior seems actually exactly what I would expect as the
caller, if I intended anything but "piggy back" semantics, which you're
adding here.


> New behavior.
> Assume no errors.
> 1. A nudge comes in with delay x and we store it.
> 2. A new nudge comes in with delay y. We will coalesce it. Then we will
> post a
> nudge at the earliest time of x and y and drop the other nudge. Before
> that We
> will destroy the pending nudge and recreate it with the new data. So when
> the
> posted job for x comes along it will not execute as the session pointer
> would be
> different.
>
> This is a cleaner implementation. Let me add some more comments and post
> another
> patch(I also found a small bug in nudge posting which is left as a todo in
> the
> code, I will fix that as well). But in the mean time we can discuss if you
> agree
> with this architecture.
>
> On 2011/12/06 20:58:45, timsteele wrote:
>
>> Actually I'm a bit confused now.  The "freshness condition" check was
>> there
>> so that nudges that came in and forced an earlier sync than the one
>> originally planned would run but the older scheduled job would not.  I
>> recall a discussion around sessions and wanting this timer reset behavior
>> which is what sparked my earlier comment, but looking at the code I
>> remember adding this check in the initial SyncerThread2 impl, because the
>> old impl would just wake up from it's loop when a new nudge came in.  It
>> seems like either your change here depends on the same freshness check, or
>> at some point the behavior regressed and that check became obsolete
>> (possibly because we now always store the pending_nudge_ variable?).
>>
>
>
>  On Tue, Dec 6, 2011 at 12:44 PM, <mailto:lipalani@chromium.org> wrote:
>>
>
>  >
>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
>> >
>>
>
> browser/sync/engine/sync_****scheduler.cc<http://**
> codereview.chromium.org/**8787006/diff/1006/chrome/**
>
browser/sync/engine/sync_**scheduler.cc<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc>
> >
>
>> > File chrome/browser/sync/engine/****sync_scheduler.cc (right):
>> >
>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
>> >
>>
>
> browser/sync/engine/sync_****scheduler.cc#newcode552<http:/**
> /codereview.chromium.org/**8787006/diff/1006/chrome/**
>
browser/sync/engine/sync_**scheduler.cc#newcode552<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc#newcode552>
> >
>
>> > chrome/browser/sync/engine/****sync_scheduler.cc:552:
>> > pending_nudge_->scheduled_****start);
>>
>> > This is a needed change and is deliberate.
>> > Need: When you put an autofill change to sync 2 hours later and then a
>> > bookmark change comes along we want to sync right now rather than
>> > waiting for 2 hours.
>> >
>> > Problem with existing impl: Since we keep pushing the new syncs to later
>> > time there is a possibility that the user's browser does not sync for
>> > extended period of time. If the user keeps opening new URLs or whatever
>> > continuously we would not be syncing ANY change for that whole time
>> > period and in general this could lead to bugs that say "my sync is not
>> > working". The new impl introduces predictability which is important at
>> > this layer.
>> >
>> > Compromise: I do believe the new impl is the way to go :) However I had
>> > a backup plan cooked up in case this is not acceptable. For autofill
>> > type of changes we need not schedule a sync at all. We can let the long
>> > poll interval take care of it. Ideally some other change would occur and
>> > sync autofill. If the user has not made any other changes it is probably
>> > OK to wait until the long poll interval. But we should hash out why we
>> > should/should not use the new logic first.
>> >
>> > On 2011/12/06 20:15:57, timsteele wrote:
>> >
>> >> This was explicitly not the goal of the implementation. Why do we need
>> >>
>> > this now?
>> >
>> >> E.g. for sessions, we have a delay of 11 seconds to coalesce. We
>> >>
>> > actually only
>> >
>> >> want to sync once things have quiesced for 11 seconds, and this would
>> >>
>> > break
>> >
>> >> that.
>> >>
>> >
>> >  In either case, shouldn't this be a separate change?
>> >>
>> >
>> >
>>
>
> http://codereview.chromium.****org/8787006/%3Chttp://coderevi**
> ew.chromium.org/8787006/ <http://codereview.chromium.org/8787006/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8787006/<http://codereview.chromium.org/8787...
>

[tim (not reviewing), 2011-12-06 23:17:53]

On Tue, Dec 6, 2011 at 3:15 PM, Tim Steele <tim@chromium.org> wrote:

> What I'm saying is the freshness condition (search sync_scheduler for that
> text) was there to handle a case where a nudge comes in with greater
> (without loss of generality) delay, we would sync, and then not want to run
> the later nudge because we just ran a sync that "took care" of what both
> nudges wanted to do.  So the fact that we don't handle this at all now is a
> regression. Fixing it is great, but you should understand what the
> freshness-check was there for and remove it if it is no longer needed.
>
> On Tue, Dec 6, 2011 at 2:37 PM, <lipalani@chromium.org> wrote:
>
>> hmm not sure I understand. Let me explain the current state and what this
>> impl
>> does:
>> Current state:
>> Let us assume there are no errors.
>> A nudge comes in with Delay x(greater than 0) so we store it.
>> A new nudge come for y(Does not matter if x < y or x > y) we will
>> coalesce x
>> with y and then drop it. then execute the nudge at x.
>>
>> So in effect we execute the first nudge that was scheduled regardless of
>> the
>> delay that was set to. Any nudges that come before the execution of this
>> nudge
>> is simply dropped.
>>
>
> It is not simply dropped.  The sessions are coalesced.
>
>> (A little bit impl detail: In DoSyncSessionJob which is the task posted to
>> execute, we compare the session pointer of the posted job parameter with
>> the
>> pending_nudge's sessions. They would be equal as pending nudge was never
>> reset)
>>
>> This is slightly confusing and could lead to a case where sync is stalled
>> for a
>> long time(if we scheduled a nudge 2 hours from now).
>>
>
> The existing behavior seems actually exactly what I would expect as the
> caller, if I intended anything but "piggy back" semantics, which you're
> adding here.
>

Well, I suppose this is arguable -- without knowing about coalescing I
might technically expect to see two nudges.

>
>
>> New behavior.
>> Assume no errors.
>> 1. A nudge comes in with delay x and we store it.
>> 2. A new nudge comes in with delay y. We will coalesce it. Then we will
>> post a
>> nudge at the earliest time of x and y and drop the other nudge. Before
>> that We
>> will destroy the pending nudge and recreate it with the new data. So when
>> the
>> posted job for x comes along it will not execute as the session pointer
>> would be
>> different.
>>
>> This is a cleaner implementation. Let me add some more comments and post
>> another
>> patch(I also found a small bug in nudge posting which is left as a todo
>> in the
>> code, I will fix that as well). But in the mean time we can discuss if
>> you agree
>> with this architecture.
>>
>> On 2011/12/06 20:58:45, timsteele wrote:
>>
>>> Actually I'm a bit confused now.  The "freshness condition" check was
>>> there
>>> so that nudges that came in and forced an earlier sync than the one
>>> originally planned would run but the older scheduled job would not.  I
>>> recall a discussion around sessions and wanting this timer reset behavior
>>> which is what sparked my earlier comment, but looking at the code I
>>> remember adding this check in the initial SyncerThread2 impl, because the
>>> old impl would just wake up from it's loop when a new nudge came in.  It
>>> seems like either your change here depends on the same freshness check,
>>> or
>>> at some point the behavior regressed and that check became obsolete
>>> (possibly because we now always store the pending_nudge_ variable?).
>>>
>>
>>
>>  On Tue, Dec 6, 2011 at 12:44 PM, <mailto:lipalani@chromium.org> wrote:
>>>
>>
>>  >
>>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
>>> >
>>>
>>
>> browser/sync/engine/sync_****scheduler.cc<http://**
>> codereview.chromium.org/**8787006/diff/1006/chrome/**
>>
browser/sync/engine/sync_**scheduler.cc<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc>
>> >
>>
>>> > File chrome/browser/sync/engine/****sync_scheduler.cc (right):
>>> >
>>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
>>> >
>>>
>>
>> browser/sync/engine/sync_****scheduler.cc#newcode552<http:/**
>> /codereview.chromium.org/**8787006/diff/1006/chrome/**
>>
browser/sync/engine/sync_**scheduler.cc#newcode552<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc#newcode552>
>> >
>>
>>> > chrome/browser/sync/engine/****sync_scheduler.cc:552:
>>> > pending_nudge_->scheduled_****start);
>>>
>>> > This is a needed change and is deliberate.
>>> > Need: When you put an autofill change to sync 2 hours later and then a
>>> > bookmark change comes along we want to sync right now rather than
>>> > waiting for 2 hours.
>>> >
>>> > Problem with existing impl: Since we keep pushing the new syncs to
>>> later
>>> > time there is a possibility that the user's browser does not sync for
>>> > extended period of time. If the user keeps opening new URLs or whatever
>>> > continuously we would not be syncing ANY change for that whole time
>>> > period and in general this could lead to bugs that say "my sync is not
>>> > working". The new impl introduces predictability which is important at
>>> > this layer.
>>> >
>>> > Compromise: I do believe the new impl is the way to go :) However I had
>>> > a backup plan cooked up in case this is not acceptable. For autofill
>>> > type of changes we need not schedule a sync at all. We can let the long
>>> > poll interval take care of it. Ideally some other change would occur
>>> and
>>> > sync autofill. If the user has not made any other changes it is
>>> probably
>>> > OK to wait until the long poll interval. But we should hash out why we
>>> > should/should not use the new logic first.
>>> >
>>> > On 2011/12/06 20:15:57, timsteele wrote:
>>> >
>>> >> This was explicitly not the goal of the implementation. Why do we need
>>> >>
>>> > this now?
>>> >
>>> >> E.g. for sessions, we have a delay of 11 seconds to coalesce. We
>>> >>
>>> > actually only
>>> >
>>> >> want to sync once things have quiesced for 11 seconds, and this would
>>> >>
>>> > break
>>> >
>>> >> that.
>>> >>
>>> >
>>> >  In either case, shouldn't this be a separate change?
>>> >>
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/8787006/%3Chttp://coderevi**
>> ew.chromium.org/8787006/ <http://codereview.chromium.org/8787006/>>
>>
>>> >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8787006/<http://codereview.chromium.org/8787...
>>
>
>

[lipalani1, 2011-12-07 22:31:02]

Summarizing everything:
Let me try to explain the expectation moving forward regardless of what the
previous expectations and regressions. Let me know your view:

When 2 nudges are scheduled and if neither of them can be executed right away
then we will choose the nudge with the earliest time and coalesce the other
nudge with this and execute it. The other nudge will simply be ignored.

The freshness condition is that if a nudge came along whose start time was
earlier than the end of time of the last executed nudge we would drop the new
nudge. It still holds true. It is generally useful if 2 nudges are scheduled
close enough(like say 2 bookmarks changes occur right away. Default nudge delay
is 200 ms. both nudges come in with delay 200ms but at times say x and x + 10
ms). In this case we execute the first nudge and discard the second because its
start time is earlier the end time of the nudge we just executed.

I have implemented this and fixed the todo I mentioned. Please review.

On 2011/12/06 23:17:53, timsteele wrote:
> On Tue, Dec 6, 2011 at 3:15 PM, Tim Steele <mailto:tim@chromium.org> wrote:
> 
> > What I'm saying is the freshness condition (search sync_scheduler for that
> > text) was there to handle a case where a nudge comes in with greater
> > (without loss of generality) delay, we would sync, and then not want to run
> > the later nudge because we just ran a sync that "took care" of what both
> > nudges wanted to do.  So the fact that we don't handle this at all now is a
> > regression. Fixing it is great, but you should understand what the
> > freshness-check was there for and remove it if it is no longer needed.
> >
> > On Tue, Dec 6, 2011 at 2:37 PM, <mailto:lipalani@chromium.org> wrote:
> >
> >> hmm not sure I understand. Let me explain the current state and what this
> >> impl
> >> does:
> >> Current state:
> >> Let us assume there are no errors.
> >> A nudge comes in with Delay x(greater than 0) so we store it.
> >> A new nudge come for y(Does not matter if x < y or x > y) we will
> >> coalesce x
> >> with y and then drop it. then execute the nudge at x.
> >>
> >> So in effect we execute the first nudge that was scheduled regardless of
> >> the
> >> delay that was set to. Any nudges that come before the execution of this
> >> nudge
> >> is simply dropped.
> >>
> >
> > It is not simply dropped.  The sessions are coalesced.
> >
> >> (A little bit impl detail: In DoSyncSessionJob which is the task posted to
> >> execute, we compare the session pointer of the posted job parameter with
> >> the
> >> pending_nudge's sessions. They would be equal as pending nudge was never
> >> reset)
> >>
> >> This is slightly confusing and could lead to a case where sync is stalled
> >> for a
> >> long time(if we scheduled a nudge 2 hours from now).
> >>
> >
> > The existing behavior seems actually exactly what I would expect as the
> > caller, if I intended anything but "piggy back" semantics, which you're
> > adding here.
> >
> 
> Well, I suppose this is arguable -- without knowing about coalescing I
> might technically expect to see two nudges.
> 
> >
> >
> >> New behavior.
> >> Assume no errors.
> >> 1. A nudge comes in with delay x and we store it.
> >> 2. A new nudge comes in with delay y. We will coalesce it. Then we will
> >> post a
> >> nudge at the earliest time of x and y and drop the other nudge. Before
> >> that We
> >> will destroy the pending nudge and recreate it with the new data. So when
> >> the
> >> posted job for x comes along it will not execute as the session pointer
> >> would be
> >> different.
> >>
> >> This is a cleaner implementation. Let me add some more comments and post
> >> another
> >> patch(I also found a small bug in nudge posting which is left as a todo
> >> in the
> >> code, I will fix that as well). But in the mean time we can discuss if
> >> you agree
> >> with this architecture.
> >>
> >> On 2011/12/06 20:58:45, timsteele wrote:
> >>
> >>> Actually I'm a bit confused now.  The "freshness condition" check was
> >>> there
> >>> so that nudges that came in and forced an earlier sync than the one
> >>> originally planned would run but the older scheduled job would not.  I
> >>> recall a discussion around sessions and wanting this timer reset behavior
> >>> which is what sparked my earlier comment, but looking at the code I
> >>> remember adding this check in the initial SyncerThread2 impl, because the
> >>> old impl would just wake up from it's loop when a new nudge came in.  It
> >>> seems like either your change here depends on the same freshness check,
> >>> or
> >>> at some point the behavior regressed and that check became obsolete
> >>> (possibly because we now always store the pending_nudge_ variable?).
> >>>
> >>
> >>
> >>  On Tue, Dec 6, 2011 at 12:44 PM, <mailto:lipalani@chromium.org> wrote:
> >>>
> >>
> >>  >
> >>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
> >>> >
> >>>
> >>
> >> browser/sync/engine/sync_****scheduler.cc<http://**
> >> codereview.chromium.org/**8787006/diff/1006/chrome/**
> >>
>
browser/sync/engine/sync_**scheduler.cc<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc>
> >> >
> >>
> >>> > File chrome/browser/sync/engine/****sync_scheduler.cc (right):
> >>> >
> >>> > http://codereview.chromium.****org/8787006/diff/1006/chrome/****
> >>> >
> >>>
> >>
> >> browser/sync/engine/sync_****scheduler.cc#newcode552<http:/**
> >> /codereview.chromium.org/**8787006/diff/1006/chrome/**
> >>
>
browser/sync/engine/sync_**scheduler.cc#newcode552<http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/engine/sync_scheduler.cc#newcode552>
> >> >
> >>
> >>> > chrome/browser/sync/engine/****sync_scheduler.cc:552:
> >>> > pending_nudge_->scheduled_****start);
> >>>
> >>> > This is a needed change and is deliberate.
> >>> > Need: When you put an autofill change to sync 2 hours later and then a
> >>> > bookmark change comes along we want to sync right now rather than
> >>> > waiting for 2 hours.
> >>> >
> >>> > Problem with existing impl: Since we keep pushing the new syncs to
> >>> later
> >>> > time there is a possibility that the user's browser does not sync for
> >>> > extended period of time. If the user keeps opening new URLs or whatever
> >>> > continuously we would not be syncing ANY change for that whole time
> >>> > period and in general this could lead to bugs that say "my sync is not
> >>> > working". The new impl introduces predictability which is important at
> >>> > this layer.
> >>> >
> >>> > Compromise: I do believe the new impl is the way to go :) However I had
> >>> > a backup plan cooked up in case this is not acceptable. For autofill
> >>> > type of changes we need not schedule a sync at all. We can let the long
> >>> > poll interval take care of it. Ideally some other change would occur
> >>> and
> >>> > sync autofill. If the user has not made any other changes it is
> >>> probably
> >>> > OK to wait until the long poll interval. But we should hash out why we
> >>> > should/should not use the new logic first.
> >>> >
> >>> > On 2011/12/06 20:15:57, timsteele wrote:
> >>> >
> >>> >> This was explicitly not the goal of the implementation. Why do we need
> >>> >>
> >>> > this now?
> >>> >
> >>> >> E.g. for sessions, we have a delay of 11 seconds to coalesce. We
> >>> >>
> >>> > actually only
> >>> >
> >>> >> want to sync once things have quiesced for 11 seconds, and this would
> >>> >>
> >>> > break
> >>> >
> >>> >> that.
> >>> >>
> >>> >
> >>> >  In either case, shouldn't this be a separate change?
> >>> >>
> >>> >
> >>> >
> >>>
> >>
> >> http://codereview.chromium.****org/8787006/%253Chttp://coderevi**
> >> ew.chromium.org/8787006/ <http://codereview.chromium.org/8787006/>>
> >>
> >>> >
> >>>
> >>
> >>
> >>
> >>
>
http://codereview.chromium.**org/8787006/%3Chttp://codereview.chromium.org/87...>
> >>
> >
> >

[tim (not reviewing), 2011-12-09 23:07:21]

Overall pretty nice CL.  A few comments.

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8787006/diff/1006/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:121: const int
SyncManager::kPiggybackNudgeDelay = 2 * 60 * 60 * 1000;
Perhaps this could be controlled via ClientCommand?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1093: ScheduleSyncSessionJob(job);
I created this function (ScheduleSyncSessionJob) taking parameters because: if
every callsite has to make this call anyway (there is actually logic happening
in ScheduleSSJ), then out of practicality (and convenience) it should be the
thing that constructs the job. It's less duplicated code everywhere, and it
should be unnatural for callsites to schedule jobs with |is_canary| set to true
because those need to be treated very carefully. Is there a functional reason
for this change?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler_unittest.cc:497: TimeDelta delay =
TimeDelta::FromSeconds(1200);
OCD nit :) I think FromDays(1) is more readable for this.  I used that elsewhere
in here for "big delays not meant to be hit".  I don't think there's anything in
test_timeouts for this type of value (or that we'd want to add it there,
necessarily).

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:121: const int
SyncManager::kPiggybackNudgeDelay = 2 * 60 * 60 * 1000;
I'm a bit confused with the relation to the periodic sync interval.  It seems
like "accompany an immediate change or wait for periodic" is a clear
standby-like behavior.  Why did you choose to separate it?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:476: TimeDelta
GetNudgeDelayTimeDeltaFromType(
I see an argument for a simple NudgeStrategy class that would encapsulate all
the code you've added here.  WDYT?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.h (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.h:63: enum NudgeDelayType {
Instead of 'Type', maybe 'Strategy' or 'Behavior'.

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.h:68:
PIGGY_BACK_WITH_ANOTHER_CHANGE,
I thought about this naming for a while since PIGGY_BACK (though pretty accurate
to those familiar with the term) wasn't my favorite.  How about ACCOMPANY,
SHADOW, TAG_ALONG, or STANDBY (much like taking a flight)?  (or ACCOMPANY_ONLY /
SHADOW_ONLY etc)

[lipalani1, 2011-12-15 01:35:15]

Addressed your feedback. Please review.

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1093: ScheduleSyncSessionJob(job);
The functional reason is in ScheduleNudgeImpl. We need to have the job there for
all the preprocessing we do before calling this function. The preprocessing
include ShouldRunJob and coalescing with the pending nudge. Once we have done
all that then we have to deconstruct to pass to this function. (one side note:
For us to deconstruct the delay we need to subtract from Now() which seems
inelegant as just few lines above that we would have added to Now() to calculate
the scheduled start).

Regarding canary_job we could have a DCHECK in ScheduleSyncSessionJob if needed.
I havent added that because that function by itself is not expected to care
about canary_job or not.

On 2011/12/09 23:07:22, timsteele wrote:
> I created this function (ScheduleSyncSessionJob) taking parameters because: if
> every callsite has to make this call anyway (there is actually logic happening
> in ScheduleSSJ), then out of practicality (and convenience) it should be the
> thing that constructs the job. It's less duplicated code everywhere, and it
> should be unnatural for callsites to schedule jobs with |is_canary| set to
true
> because those need to be treated very carefully. Is there a functional reason
> for this change?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler_unittest.cc:497: TimeDelta delay =
TimeDelta::FromSeconds(1200);
On 2011/12/09 23:07:22, timsteele wrote:
> OCD nit :) I think FromDays(1) is more readable for this.  I used that
elsewhere
> in here for "big delays not meant to be hit".  I don't think there's anything
in
> test_timeouts for this type of value (or that we'd want to add it there,
> necessarily).

Done.

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:121: const int
SyncManager::kPiggybackNudgeDelay = 2 * 60 * 60 * 1000;
I went back and forth. The advantage is that if it is not tied to polling
interval we could control this independently(like via a client command etc).
However I think unifying them makes more sense. If we actually find a use case
where they have to be changed dynamically we can change to it. But for now
unifying them reduces a variable and efficient sync wise. I am using the short
polling interval here in order to not wait too long.
On 2011/12/09 23:07:22, timsteele wrote:
> I'm a bit confused with the relation to the periodic sync interval.  It seems
> like "accompany an immediate change or wait for periodic" is a clear
> standby-like behavior.  Why did you choose to separate it?

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:476: TimeDelta
GetNudgeDelayTimeDeltaFromType(
On 2011/12/09 23:07:22, timsteele wrote:
> I see an argument for a simple NudgeStrategy class that would encapsulate all
> the code you've added here.  WDYT?

Done.

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.h (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.h:63: enum NudgeDelayType {
On 2011/12/09 23:07:22, timsteele wrote:
> Instead of 'Type', maybe 'Strategy' or 'Behavior'.

Done.

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.h:68:
PIGGY_BACK_WITH_ANOTHER_CHANGE,
On 2011/12/09 23:07:22, timsteele wrote:
> I thought about this naming for a while since PIGGY_BACK (though pretty
accurate
> to those familiar with the term) wasn't my favorite.  How about ACCOMPANY,
> SHADOW, TAG_ALONG, or STANDBY (much like taking a flight)?  (or ACCOMPANY_ONLY
/
> SHADOW_ONLY etc)

Done.

[tim (not reviewing), 2011-12-15 20:59:15]

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:121: const int
SyncManager::kPiggybackNudgeDelay = 2 * 60 * 60 * 1000;
On 2011/12/15 01:35:16, lipalani1 wrote:
> I went back and forth. The advantage is that if it is not tied to polling
> interval we could control this independently(like via a client command etc).
> However I think unifying them makes more sense. If we actually find a use case
> where they have to be changed dynamically we can change to it. But for now
> unifying them reduces a variable and efficient sync wise. I am using the short
> polling interval here in order to not wait too long.
> On 2011/12/09 23:07:22, timsteele wrote:
> > I'm a bit confused with the relation to the periodic sync interval.  It
seems
> > like "accompany an immediate change or wait for periodic" is a clear
> > standby-like behavior.  Why did you choose to separate it?
> 

Short-poll sounds good, but shouldn't we tie it to the actual variable, not
value, since it can change via ClientCommand

[lipalani1, 2011-12-15 21:12:35]

That was deliberate. My Initial version had a function to query from the
scheduler.

Then I changed it to this for predictability. The server could bump the value
anytime and whenever the autofill does not sync we would not  know if it is
because of a bug or because of server bumping the polling rate.

Also if the server bumps the value to a high value thinking it affects only
polling we would be left with too many unsynced autofills.

Also in most cases we expect a sync to take place before this. So this unifying
is mostly for code efficiency rather than reducing actual syncs.
On 2011/12/15 20:59:15, timsteele wrote:
>
http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
> File chrome/browser/sync/internal_api/sync_manager.cc (right):
> 
>
http://codereview.chromium.org/8787006/diff/11001/chrome/browser/sync/interna...
> chrome/browser/sync/internal_api/sync_manager.cc:121: const int
> SyncManager::kPiggybackNudgeDelay = 2 * 60 * 60 * 1000;
> On 2011/12/15 01:35:16, lipalani1 wrote:
> > I went back and forth. The advantage is that if it is not tied to polling
> > interval we could control this independently(like via a client command etc).
> > However I think unifying them makes more sense. If we actually find a use
case
> > where they have to be changed dynamically we can change to it. But for now
> > unifying them reduces a variable and efficient sync wise. I am using the
short
> > polling interval here in order to not wait too long.
> > On 2011/12/09 23:07:22, timsteele wrote:
> > > I'm a bit confused with the relation to the periodic sync interval.  It
> seems
> > > like "accompany an immediate change or wait for periodic" is a clear
> > > standby-like behavior.  Why did you choose to separate it?
> > 
> 
> Short-poll sounds good, but shouldn't we tie it to the actual variable, not
> value, since it can change via ClientCommand

[tim (not reviewing), 2011-12-16 02:52:10]

LGTM with nits

http://codereview.chromium.org/8787006/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/8787006/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:513: ScheduleSyncSessionJob(job);
suggestion here and elsewhere, you could just inline it
ScheduleSyncSessionJob(SyncSessionJob(..., ..., ...))

http://codereview.chromium.org/8787006/diff/20001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8787006/diff/20001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:118: const int
SyncManager::kDefaultNudgeDelayMilliseconds = 200;
seems like these should go along with the other constants in polling_constants. 
Following the spirit of that file, not necessary the letter...

http://codereview.chromium.org/8787006/diff/20001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:577: ACCOMPANY_ONLY,
Based on the fact that we actually _will_ sync it later (independently) I guess
the _ONLY suffix was a bad suggestion on my part. ACCOMPANY seems good though. 
Or STANDBY.

[lipalani1, 2011-12-17 00:00:58]

Hey George this change is LGTMed. However it might be a good idea for you to
take a look at the two_client_autofill_sync_test.cc and LGTM it if it looks ok.

[GeorgeY, 2011-12-17 00:07:31]

LGTM (when bots succeed)

[Raghu Simha, 2011-12-17 00:19:39]

I have one drive by comment. Feel free to do what's best.

http://codereview.chromium.org/8787006/diff/30001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc
(right):

http://codereview.chromium.org/8787006/diff/30001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:37: // By
default autofill does not sync unless there is some other change.
Curious: Instead of calling this function, why not just add a bookmark? I think
AddURL(<profile>, IndexedURL(i)) will have the same effect as this function.

[commit-bot: I haz the power, 2011-12-18 07:10:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8787006/30001

[commit-bot: I haz the power, 2011-12-18 16:28:28]

Change committed as 114941

[lipalani1, 2011-12-19 20:41:29]

Please review.

[Raghu Simha, 2011-12-19 20:50:48]

A few comments below.

Also, it looks like the latest patch set is missing your original patch, so
you'll probably need to pull down your original patch, apply this, and re-upload
the merged patch.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc
(right):

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:28: using
bookmarks_helper::AddFolder;
Remove this.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:32: using
bookmarks_helper::SetTitle;
Remove this.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:36:
TwoClientAutofillSyncTest() : SyncTest(TWO_CLIENT) {count = 0;}
nit: I think the style guide recommends spaces after / before the open / close
brace.

[lipalani1, 2011-12-19 20:54:01]

Please review.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc
(right):

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:28: using
bookmarks_helper::AddFolder;
On 2011/12/19 20:50:48, rsimha wrote:
> Remove this.

Done.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:32: using
bookmarks_helper::SetTitle;
On 2011/12/19 20:50:48, rsimha wrote:
> Remove this.

Done.

http://codereview.chromium.org/8787006/diff/46002/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:36:
TwoClientAutofillSyncTest() : SyncTest(TWO_CLIENT) {count = 0;}
On 2011/12/19 20:50:48, rsimha wrote:
> nit: I think the style guide recommends spaces after / before the open / close
> brace.

Done.

[Raghu Simha, 2011-12-19 21:01:58]

One more fix, then rebase on your original patch, then LGTM.

http://codereview.chromium.org/8787006/diff/20004/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc
(right):

http://codereview.chromium.org/8787006/diff/20004/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc:42: }
nit: fix indent.

[lipalani1, 2011-12-19 21:12:24]

all the hacky methods I spoke to you offline seem to be running into issues
:(

So decided to do it the right way by going back in history, redownloading
etc should be checked in after the try runs.
thanks!

On Mon, Dec 19, 2011 at 1:01 PM, <rsimha@chromium.org> wrote:

> One more fix, then rebase on your original patch, then LGTM.
>
>
> http://codereview.chromium.**org/8787006/diff/20004/chrome/**
>
browser/sync/test/integration/**two_client_autofill_sync_test.**cc<http://codereview.chromium.org/8787006/diff/20004/chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc>
> File
> chrome/browser/sync/test/**integration/two_client_**autofill_sync_test.cc
> (right):
>
> http://codereview.chromium.**org/8787006/diff/20004/chrome/**
> browser/sync/test/integration/**two_client_autofill_sync_test.**
>
cc#newcode42<http://codereview.chromium.org/8787006/diff/20004/chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc#newcode42>
> chrome/browser/sync/test/**integration/two_client_**
> autofill_sync_test.cc:42:
> }
> nit: fix indent.
>
>
http://codereview.chromium.**org/8787006/<http://codereview.chromium.org/8787...
>

[commit-bot: I haz the power, 2011-12-20 01:22:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8787006/20007

[commit-bot: I haz the power, 2011-12-20 02:43:34]

Change committed as 115072