Avoid SYNC_CYCLE_CONTINUATION sync cycles

Avoid SYNC_CYCLE_CONTINUATION sync cycles

This is part 3 of 3 in a series of patches to remove the unnecessary
SYNC_CYCLE_CONTINUATION sync cycle that follows every commit.

Previously, we would schedule a SYNC_CYCLE_CONTINUATION sync cycle to
retry in case of failure if we detected that the number of unsynced handles
was greater than zero by the time the current sync cycle was complete.
That value was stale; it was updated only at the beginning of the sync
cycle and did not take into account the fact that many handles may have
been committed during the cycle.  Every cycle that started with at least
one uncommitted item would result in an extra SYNC_CYCLE_CONTINUATION
(and GetUpdates request to the server) being performed.

The new logic attempts to perform SYNC_CYCLE_CONTINUATION sync cycles only
when necessary.  The logic is based on the idea that we should only retry
if we detect a failure.  Parts 1 and 2 in this patch series provide a
mechanism for detecting failures.  This patch makes it so we schedule
another sync cycle only if that code detects a failure.

Note that the logic being used here does not distinguish between
transient an non-transient errors.  We could optimize this code by not
retrying unless we have reason to believe a retry would be a sensible
response to this kind of error.  However, the old code did not attempt
to handle different errors differently, so we won't either.  The
exponential backoff in SyncScheduler should ensure this doesn't cause
any serious problems.

BUG=94670
TEST=Manual.  Trigger sync for a Zipit data type with about:sync open.
Note 'Empty GetUpdates' counter is not incremented with each change.


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

[rlarocque, 2012-01-11 22:37:36]

Here is the last patch in the series.  

Parts 1 and 2 are at http://codereview.chromium.org/9113024/ and
http://codereview.chromium.org/9158004/, respectively.  The squashed patch
series is available for viewing at http://codereview.chromium.org/9036003/.

[rlarocque, 2012-01-11 23:01:52]

On 2012/01/11 22:37:36, rlarocque wrote:
> Here is the last patch in the series.  
> 
> Parts 1 and 2 are at http://codereview.chromium.org/9113024/ and
> http://codereview.chromium.org/9158004/, respectively.  The squashed patch
> series is available for viewing at http://codereview.chromium.org/9036003/.

You can safely ignore the try failures in the first set of trybots.  They failed
because they were built against a rev that preceded the commit of patch 2 in
this series.  I'll try again later.

[tim (not reviewing), 2012-01-12 01:36:59]

On 2012/01/11 23:01:52, rlarocque wrote:
> On 2012/01/11 22:37:36, rlarocque wrote:
> > Here is the last patch in the series.  
> > 
> > Parts 1 and 2 are at http://codereview.chromium.org/9113024/ and
> > http://codereview.chromium.org/9158004/, respectively.  The squashed patch
> > series is available for viewing at http://codereview.chromium.org/9036003/.
> 
> You can safely ignore the try failures in the first set of trybots.  They
failed
> because they were built against a rev that preceded the commit of patch 2 in
> this series.  I'll try again later.

OK. Note you can use 'git try -r' if you want to bypass LKGR.

[tim (not reviewing), 2012-01-12 01:42:31]

lgtm

http://codereview.chromium.org/9185031/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_scheduler.cc (left):

http://codereview.chromium.org/9185031/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:859: // TODO(tim): Old impl had
special code if notifications disabled. Needed?
Curious, did you look this up? IIRC there was a chunk of code that I basically
just completely ignored when writing the original SyncScheduler.

[rlarocque, 2012-01-12 19:43:33]

http://codereview.chromium.org/9185031/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_scheduler.cc (left):

http://codereview.chromium.org/9185031/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:859: // TODO(tim): Old impl had
special code if notifications disabled. Needed?
On 2012/01/12 01:42:31, timsteele wrote:
> Curious, did you look this up? IIRC there was a chunk of code that I basically
> just completely ignored when writing the original SyncScheduler.

I haven't looked at it too much.  I've seen the code, but it references a lot of
variables that don't mean a lot to me.

That comment was written a year ago.  If removing that code caused bugs, we
probably would have heard about it by now.

[lipalani1, 2012-01-13 00:00:30]

LGTM but i think there could be more test failures. if I remember right we had
lots of tests testing exponential backoff.

[rlarocque, 2012-01-20 06:45:13]

+Fred

As you can see, there were integration test failures.

Some of these were due to the way explicit passphrases were interacting with the
ProfileSyncServiceHarness.  See crbug.com/110529 and r118387.

The other failures are due to self-notifications not entirely making up for the
lack of extra sync cycles.  I added some infrastructure to "fix" these a few
months ago, but more tests have been added since then.

Fred: git blame tells me you were the original author of many of these tests I'm
modifying.  Could you review my changes to them?  I think the delta from patch
set 2 to patch set 4 would be a good place to start, though you're welcome to
review the other changes, too.

[akalin, 2012-01-20 22:41:51]

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:46:
DisableNotifications();
why was this necessary?  i.e. why does setting up sync for a single client need
notifications to be enabled?  that seems weird.

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:79:
DisableNotifications();
why was this necessary?

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/migration_errors_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/migration_errors_test.cc:175: //
AwaitQuiescence() will not succeed when notifications are disabled.  We
Why is this true?  It seems to me that a single client should be trivially
"quiesced" already.

[rlarocque, 2012-01-20 23:04:56]

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:46:
DisableNotifications();
On 2012/01/20 22:41:51, akalin wrote:
> why was this necessary?  i.e. why does setting up sync for a single client
need
> notifications to be enabled?  that seems weird.

The problem is that SetupSync() doesn't complete until the profile can pass the
IsFullySynced() test.  In order for that to happen, the harness has to see a
snapshot with num_unsynced_handles == 0, or, in other words, a non-committing
sync cycle.  The SYNC_CYCLE_CONTINUATION used to ensure this would always
happen.  I'm removing that (otherwise useless) sync cycle, so we need something
else to trigger a useless sync cycle to keep the integration tests happy.  

It might also be possible to make this work by relaxing the constraint in
SetupSync() to be IsDataSynced() rather than IsFullySynced().  However, I think
I tried that a while a ago and found it interacted badly with other tests.  This
seems like the easiest way to make things work for now, without significantly
reducing the coverage of the integration tests.  

See also crbug.com/97780, which tracks the efforts to fix these hacks.

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:79:
DisableNotifications();
On 2012/01/20 22:41:51, akalin wrote:
> why was this necessary?

Same reasoning as above.

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/migration_errors_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/migration_errors_test.cc:175: //
AwaitQuiescence() will not succeed when notifications are disabled.  We
On 2012/01/20 22:41:51, akalin wrote:
> Why is this true?  It seems to me that a single client should be trivially
> "quiesced" already.

Not according to the definition in the integration tests.  It requires that all
clients be "IsFullySynced()", which won't happen automatically when there are no
self-notifications or SYNC_CYCLE_CONTINUATION GetUpdates.

I would like to fix this by relaxing the constraints for AwaitQuiescense(), but
that would break other tests that rely on comparison of timestamps.  In general,
we can't expect timestamps to match unless this clients being compared are both
IsFullySynced().

[akalin, 2012-01-20 23:07:24]

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:46:
DisableNotifications();
On 2012/01/20 23:04:56, rlarocque wrote:
> On 2012/01/20 22:41:51, akalin wrote:
> > why was this necessary?  i.e. why does setting up sync for a single client
> need
> > notifications to be enabled?  that seems weird.
> 
> The problem is that SetupSync() doesn't complete until the profile can pass
the
> IsFullySynced() test.  In order for that to happen, the harness has to see a
> snapshot with num_unsynced_handles == 0, or, in other words, a non-committing
> sync cycle.  The SYNC_CYCLE_CONTINUATION used to ensure this would always
> happen.  I'm removing that (otherwise useless) sync cycle, so we need
something
> else to trigger a useless sync cycle to keep the integration tests happy.  
> 
> It might also be possible to make this work by relaxing the constraint in
> SetupSync() to be IsDataSynced() rather than IsFullySynced().  However, I
think
> I tried that a while a ago and found it interacted badly with other tests. 
This
> seems like the easiest way to make things work for now, without significantly
> reducing the coverage of the integration tests.  
> 
> See also crbug.com/97780, which tracks the efforts to fix these hacks.

Okay, if this is a temporary change, add a TODO to move it back and refer to the
bug.

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:79:
DisableNotifications();
On 2012/01/20 23:04:56, rlarocque wrote:
> On 2012/01/20 22:41:51, akalin wrote:
> > why was this necessary?
> 
> Same reasoning as above.

add todo.

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/migration_errors_test.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/migration_errors_test.cc:175: //
AwaitQuiescence() will not succeed when notifications are disabled.  We
On 2012/01/20 23:04:56, rlarocque wrote:
> On 2012/01/20 22:41:51, akalin wrote:
> > Why is this true?  It seems to me that a single client should be trivially
> > "quiesced" already.
> 
> Not according to the definition in the integration tests.  It requires that
all
> clients be "IsFullySynced()", which won't happen automatically when there are
no
> self-notifications or SYNC_CYCLE_CONTINUATION GetUpdates.
> 
> I would like to fix this by relaxing the constraints for AwaitQuiescense(),
but
> that would break other tests that rely on comparison of timestamps.  In
general,
> we can't expect timestamps to match unless this clients being compared are
both
> IsFullySynced().

Add TODO to change this back and refer to bug.

[akalin, 2012-01-20 23:09:29]

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/9185031/diff/19001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:658: // rely on
self-notifiations to ensure that timestamps are udpated, which
self-notifiations -> self-notifications

[rlarocque, 2012-01-20 23:20:48]

New patch uploaded.  PTAL.

[akalin, 2012-01-20 23:25:08]

LGTM

[commit-bot: I haz the power, 2012-01-20 23:26:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9185031/22004

[commit-bot: I haz the power, 2012-01-21 00:53:18]

Change committed as 118572