sync: Add pre-commit update avoidance mode + flag

sync: Add pre-commit update avoidance mode + flag

This CL allows the syncer to avoid requesting updates from the server
prior to a commit if the nudge tracker determines that such a request is
not necessary.  In general, the nudge tracker will consider such a
request to be unnecessary unless the sync cycle was triggered by a local
refresh request or an invalidation, or invalidations are disabled.

This behavior is enabled only if explicitly requested by the client or
the server.  The client can enable this mode with the command-line flag
'--sync-enable-get-update-avoidance'.  The server can toggle it on or
off with the 'pre_commit_update_avoidance' experiment.  Unless at least
one of these flags are set, this CL will have no effect on syncer
behavior.

When this experimental mode is enabled, the SyncScheduler will use the
"short poll" interval rather than the "long poll".  This should help
clients keep in sync with each other even if the experiment breaks.

Also included in this CL are some extensions to the python test server
to enable manual testing of the pre_commit_update_avoidance experiment
node.

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

[rlarocque, 2013-07-15 23:56:29]

zea: Please review.
raz: Please skim and let me know if there's anything you'd like changed to make
this easier to use server-side.

[Nicolas Zea, 2013-07-16 00:24:32]

https://codereview.chromium.org/19309002/diff/1/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/19309002/diff/1/sync/engine/syncer.cc#newcode68
sync/engine/syncer.cc:68: ||
session->context()->should_fetch_updates_before_commit()) {
|| on previous line

https://codereview.chromium.org/19309002/diff/1/sync/sessions/nudge_tracker.h
File sync/sessions/nudge_tracker.h (right):

https://codereview.chromium.org/19309002/diff/1/sync/sessions/nudge_tracker.h...
sync/sessions/nudge_tracker.h:37: // Returns true if there is a good reason for
performan a get updates
performan -> performing

https://codereview.chromium.org/19309002/diff/1/sync/sessions/sync_session_co...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/19309002/diff/1/sync/sessions/sync_session_co...
sync/sessions/sync_session_context.h:197: bool
pre_commit_update_avoidance_experiment_enabled_;
Do we plan for this to stick around (i.e. is the eventual state for whether we
do pre-commit GU going to remain server controlled)? I wonder if we shouldn't
call it an experiment then.

https://codereview.chromium.org/19309002/diff/1/sync/tools/testserver/chromiu...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/19309002/diff/1/sync/tools/testserver/chromiu...
sync/tools/testserver/chromiumsync.py:134:
PRE_COMMIT_GU_AVOIDANCE_EXPERIMENT_TAG = "Z1xgeh3QUBa50vdEPd8C/4c7jfE="
Newline above this and add comment (it's not associated with encryption
experiment. Alternatively, update the above comment to be hashed client tag for
experiments.

[rlarocque, 2013-07-16 01:08:26]

Patch updated.  PTAL.

https://codereview.chromium.org/19309002/diff/1/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/19309002/diff/1/sync/engine/syncer.cc#newcode68
sync/engine/syncer.cc:68: ||
session->context()->should_fetch_updates_before_commit()) {
On 2013/07/16 00:24:32, Nicolas Zea wrote:
> || on previous line

Done.

https://codereview.chromium.org/19309002/diff/1/sync/sessions/nudge_tracker.h
File sync/sessions/nudge_tracker.h (right):

https://codereview.chromium.org/19309002/diff/1/sync/sessions/nudge_tracker.h...
sync/sessions/nudge_tracker.h:37: // Returns true if there is a good reason for
performan a get updates
On 2013/07/16 00:24:32, Nicolas Zea wrote:
> performan -> performing

Done.

https://codereview.chromium.org/19309002/diff/1/sync/sessions/sync_session_co...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/19309002/diff/1/sync/sessions/sync_session_co...
sync/sessions/sync_session_context.h:197: bool
pre_commit_update_avoidance_experiment_enabled_;
On 2013/07/16 00:24:32, Nicolas Zea wrote:
> Do we plan for this to stick around (i.e. is the eventual state for whether we
> do pre-commit GU going to remain server controlled)? I wonder if we shouldn't
> call it an experiment then.

Good point.  I was hoping to remove this flag and make this behaviour the
default eventually, but it could be a long time before we get to that point.  I
could go either way on the name.  

Raz, any thoughts?

https://codereview.chromium.org/19309002/diff/1/sync/tools/testserver/chromiu...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/19309002/diff/1/sync/tools/testserver/chromiu...
sync/tools/testserver/chromiumsync.py:134:
PRE_COMMIT_GU_AVOIDANCE_EXPERIMENT_TAG = "Z1xgeh3QUBa50vdEPd8C/4c7jfE="
On 2013/07/16 00:24:32, Nicolas Zea wrote:
> Newline above this and add comment (it's not associated with encryption
> experiment. Alternatively, update the above comment to be hashed client tag
for
> experiments.

Done.

[rlarocque, 2013-07-18 00:10:15]

I talked to Raz.  He also thinks that using "Experiment" as part of the name is
a bad idea.

I've updated the CL.  The SessionContext now has
'server_enabled_pre_commit_update_avoidance_' and
'client_enabled_pre_commit_update_avoidance_' flags.

PTAL.

[rlarocque, 2013-07-19 19:28:03]

Hi Tim, could you take a look at this while Nicolas is out?

[tim (not reviewing), 2013-07-19 20:11:36]

LGTM with nits.

https://codereview.chromium.org/19309002/diff/20001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/19309002/diff/20001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_unittest.cc:153: false,  // force enable pre-commit
GU avoidance experiment
nit - 'experiment'?

https://codereview.chromium.org/19309002/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/internal_components_factory.h (right):

https://codereview.chromium.org/19309002/diff/20001/sync/internal_api/public/...
sync/internal_api/public/internal_components_factory.h:71:
PreCommitUpdatesPolicy pre_commit_updates_policy_override;
'override' seems superfluous in the name here.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/data_type_t...
File sync/sessions/data_type_tracker.cc (right):

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/data_type_t...
sync/sessions/data_type_tracker.cc:70: !pending_payloads_.empty() ||
HasPendingInvalidation() would actually be more readable here.  That or rename
pending_payloads to be pending_invalidation_payloads... although I still think
HasPendingInvalidation wins.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:133: bool
should_fetch_updates_before_commit() const {
nit - Style Guide Says: Use CamelCase for this versus unix_hacker.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:201: bool
client_enabled_pre_commit_update_avoidance_;
This one should be const.

[rlarocque, 2013-07-19 20:35:46]

https://codereview.chromium.org/19309002/diff/20001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/19309002/diff/20001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_unittest.cc:153: false,  // force enable pre-commit
GU avoidance experiment
On 2013/07/19 20:11:36, timsteele wrote:
> nit - 'experiment'?

Done.

https://codereview.chromium.org/19309002/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/internal_components_factory.h (right):

https://codereview.chromium.org/19309002/diff/20001/sync/internal_api/public/...
sync/internal_api/public/internal_components_factory.h:71:
PreCommitUpdatesPolicy pre_commit_updates_policy_override;
On 2013/07/19 20:11:36, timsteele wrote:
> 'override' seems superfluous in the name here.  

Done.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/data_type_t...
File sync/sessions/data_type_tracker.cc (right):

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/data_type_t...
sync/sessions/data_type_tracker.cc:70: !pending_payloads_.empty() ||
On 2013/07/19 20:11:36, timsteele wrote:
> HasPendingInvalidation() would actually be more readable here.  That or rename
> pending_payloads to be pending_invalidation_payloads... although I still think
> HasPendingInvalidation wins.

Done.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:133: bool
should_fetch_updates_before_commit() const {
On 2013/07/19 20:11:36, timsteele wrote:
> nit - Style Guide Says: Use CamelCase for this versus unix_hacker.

Done.

https://codereview.chromium.org/19309002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:201: bool
client_enabled_pre_commit_update_avoidance_;
On 2013/07/19 20:11:36, timsteele wrote:
> This one should be const.

Done.

[commit-bot: I haz the power, 2013-07-19 20:41:17]

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

[commit-bot: I haz the power, 2013-07-20 05:35:37]

Change committed as 212746