sync: Support non-blocking initial sync in proto

sync: Support non-blocking initial sync in proto

Adds support for non-blocking initial sync to the sync protocol.  This
allows some types to request an initial sync without having the
scheduler enter configure mode.  The protocol changes are necessary to
keep the server informed that the request is due to an initial sync
request.

The remainder of the changes in this CL were required to plumb
information about the cause of the sync cycle through the SyncScheduler.
This CL also includes some new tests.

At the moment, there are no types in use that support this feature.
This CL will not affect behavior in any way.

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

[rlarocque, 2014-07-16 23:53:20]

Here's the protocol change for initial sync that I've been talking about on the
mailing list for the past few days.

Please review.

[Nicolas Zea, 2014-07-17 16:58:08]

Mostly LG, with a question

https://codereview.chromium.org/387983004/diff/20001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/387983004/diff/20001/sync/protocol/sync.proto...
sync/protocol/sync.proto:490: optional bool initial_sync_in_progress = 7;
I wonder, should we specify which types are requesting the initial sync? It
might be useful for the backend to e.g. know which backing stores it actually
needs to hit.

[stanisc, 2014-07-17 18:46:10]

https://codereview.chromium.org/387983004/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/387983004/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:401: void
SyncSchedulerImpl::ScheduleInitialSyncNudge(syncer::ModelType model_type) {
This is called ScheduleInitialSyncNudge. Does the "initial" part imply that this
should be called only once per model_type? Would calling this multiple times be
a problem?

[rlarocque, 2014-07-17 18:58:06]

https://codereview.chromium.org/387983004/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/387983004/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:401: void
SyncSchedulerImpl::ScheduleInitialSyncNudge(syncer::ModelType model_type) {
On 2014/07/17 18:46:10, stanisc wrote:
> This is called ScheduleInitialSyncNudge. Does the "initial" part imply that
this
> should be called only once per model_type? Would calling this multiple times
be
> a problem?

It should be very uncommon that it gets called more than once per ModelType, but
not impossible.  

We should to call it every time we need to ask the server to re-sync our type
when we're starting from nothing.  This will happen when the user first signs
in.  It can also happen if we've deleted our local sync data because a user
disabled then re-enabled sync for this type.  Or it could be that the server
requested a "migration", which is to say that it asked the client to clear its
sync data and start syncing from scratch.

The effects of calling it unnecessarily won't be too severe.  It would cause the
client to perform a GetUpdate request.  The server would (1) be forced to read
from its storage, since it is not allowed to "optimize" requests that carry this
flag, and (2) log this as an initial sync request, which might have an impact on
our graphs or metrics.

We certainly don't want to call it for no reason, but it's reasonably safe in
that there would be no data corruption if we did.

https://codereview.chromium.org/387983004/diff/20001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/387983004/diff/20001/sync/protocol/sync.proto...
sync/protocol/sync.proto:490: optional bool initial_sync_in_progress = 7;
On 2014/07/17 16:58:08, Nicolas Zea wrote:
> I wonder, should we specify which types are requesting the initial sync? It
> might be useful for the backend to e.g. know which backing stores it actually
> needs to hit.

The triggers are per-type.  I think they're co-located with the progress
markers.

All the data required to gather per-type stats are here.  Whether or not it's
easy to turn it into useful graphs on the server may be another matter, though.

[Nicolas Zea, 2014-07-17 19:02:44]

Ah, missed that. LGTM then

[rlarocque, 2014-07-17 19:54:40]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:57:38]

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

[commit-bot: I haz the power, 2014-07-17 23:44:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-18 00:06:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:06:45]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[rlarocque, 2014-07-18 00:08:12]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:12:52]

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

[commit-bot: I haz the power, 2014-07-18 00:35:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-18 00:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:55:56]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[rlarocque, 2014-07-18 18:07:54]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:08:25]

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

[commit-bot: I haz the power, 2014-07-18 18:13:48]

Change committed as 284159