[Sync] Add support for retrying a getupdates due to a context change

[Sync] Add support for retrying a getupdates due to a context change

We detect a context conflict using the version value, and if detected force the
getupdates to retry.

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

[Nicolas Zea, 2014-04-10 00:57:49]

PTAL. This assumes that the server, if it cares about always getting the most
recent context (and support refreshes) returns the context sent up with the
getupdates. Otherwise it can just return an unset context.

[rlarocque, 2014-04-10 01:15:29]

This looks OK, but I would like to see some more tests.

https://codereview.chromium.org/232003005/diff/1/sync/engine/get_updates_proc...
File sync/engine/get_updates_processor.cc (right):

https://codereview.chromium.org/232003005/diff/1/sync/engine/get_updates_proc...
sync/engine/get_updates_processor.cc:338: if
(!update_handler_iter->second->ProcessGetUpdatesResponse(
Could we have ProcessGetUpdatesResponse return an explicit error code?  I think
this code would be clearer if it returned a SyncerError enum..

Inferring that a false return value means DATATYPE_TRIGGERED_RETRY seems
brittle.

[Nicolas Zea, 2014-04-10 19:06:26]

PTAL

https://codereview.chromium.org/232003005/diff/1/sync/engine/get_updates_proc...
File sync/engine/get_updates_processor.cc (right):

https://codereview.chromium.org/232003005/diff/1/sync/engine/get_updates_proc...
sync/engine/get_updates_processor.cc:338: if
(!update_handler_iter->second->ProcessGetUpdatesResponse(
On 2014/04/10 01:15:29, rlarocque wrote:
> Could we have ProcessGetUpdatesResponse return an explicit error code?  I
think
> this code would be clearer if it returned a SyncerError enum..
> 
> Inferring that a false return value means DATATYPE_TRIGGERED_RETRY seems
> brittle.

Done.

[rlarocque, 2014-04-10 20:02:35]

LGTM with one more suggestion.

https://codereview.chromium.org/232003005/diff/40001/sync/engine/directory_up...
File sync/engine/directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/232003005/diff/40001/sync/engine/directory_up...
sync/engine/directory_update_handler_unittest.cc:355: sync_pb::DataTypeContext
new_context;
Could this second half be made into a separate test?

[Nicolas Zea, 2014-04-10 20:44:21]

Commiting via commitbot

https://codereview.chromium.org/232003005/diff/40001/sync/engine/directory_up...
File sync/engine/directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/232003005/diff/40001/sync/engine/directory_up...
sync/engine/directory_update_handler_unittest.cc:355: sync_pb::DataTypeContext
new_context;
On 2014/04/10 20:02:36, rlarocque wrote:
> Could this second half be made into a separate test?

Not really. The first half is mainly setting up the second half (by setting
data, so we can verify it doesn't get overwritten incorrectly).

[Nicolas Zea, 2014-04-10 20:44:25]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/232003005/40001

[commit-bot: I haz the power, 2014-04-10 23:57:59]

Change committed as 263134