sync: Report GetUpdate triggers to the server

sync: Report GetUpdate triggers to the server

This commit makes use of the protocol buffers added in r198250.

This adds code to the nudge tracker so it can keep more detailed
information about the reasons for fetching an update.  This information
is copied into the per-type GetUpdateTriggers when we perform a
"triggered" GetUpdate.

There was no channel available to pass the nudge_tracker through to the
DownloadUpdatesCommand, so it ended up being passed through a
(maybe-NULL) member of the SyncSession.  That's part of the reason why
this change ended up touching so many files.

The other reason why this change is so big is testing.  Lots of tests
had to be updated with some amended for the new SyncScheduler function
signatures.  This commit also includes quite a few test cases for the
NudgeTracker itself.

Because this change was delayed longer than expected, I've updated the
comments in sync.proto referencing M28 to reference M29 instead.

BUG=231693, 138613
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199136

[rlarocque, 2013-05-06 18:21:28]

Here's the follow-up to the GUTriggers protobuf change.  I've augmented the
NudgeTracker to populate most of those new fields.  (The server-side trickles
support is stubbed out for now.)

The way the NudgeTracker gets passed around is a bit ugly right now.  I plan to
clean it up when I modify the Syncer's interface so we can start scheduling
Download-only or Commit-only sync cycles.  

Please review.

[tim (not reviewing), 2013-05-06 21:29:04]

Some style nits, have to run to a meeting but will continue review afterwards.

https://codereview.chromium.org/14963002/diff/2001/sync/engine/download_updat...
File sync/engine/download_updates_command.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/engine/download_updat...
sync/engine/download_updates_command.cc:129:
DCHECK_EQ(sync_pb::SyncEnums::GU_TRIGGER, origin);
The real question we're asking is "What kind of GU is being requested here?". It
would flow more clearly if we ask that, rather than inferring it.

if (origin == sync_pb::SyncEnums::GU_TRIGGER) {
  session->nudge_tracker()->FillProtoMessage(
      ...;
} else {
  DCHECK(!session->nudge_tracker());
}

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:387: void
SyncSchedulerImpl::ScheduleInvalidation(
ScheduleInvalidationNudge seems better, to distinguish it from something that
sounds like it might call directly into ticl.

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:685: } else if (mode_ == NORMAL_MODE &&
nudge_tracker_.SyncRequired()) {
Tangential question, would NORMAL_MODE be better renamed to NUDGE_MODE?

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:14: size_t NudgeTracker::kMaxPayloadsPerType =
10;
Might this be worth adding to client command as something we can override via
cdd?

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:104: SyncSourceInfo NudgeTracker::source_info()
const {
This isn't such a trivial method.  GetSourceInfo() is a better name as per style
guide.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:114: ModelTypeSetToInvalidationMap(nudged_types,
"");
prefer std::string() to "" for efficiency & explicitness (vs implicit
constructor).

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

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.h:34: bool SyncRequired();
nit - IsSyncRequired()

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.h:52: void InvalidationsEnabled();
Convention in Chrome for something like this would be to name it
OnInvalidationsEnabled/Disabled.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/sync_session...
sync/sessions/sync_session.h:98: SyncSession(SyncSessionContext* context,
Having factory methods with appropriate names that explain the purpose (e.g
BuildSessionForNudge) would be preferable to overloading the constructor like
this (for similar reasons to why the style guide forbids overloading functions).
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[rlarocque, 2013-05-07 18:45:39]

Thanks for the quick response.

New patch uploaded, PTAL.

https://codereview.chromium.org/14963002/diff/2001/sync/engine/download_updat...
File sync/engine/download_updates_command.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/engine/download_updat...
sync/engine/download_updates_command.cc:129:
DCHECK_EQ(sync_pb::SyncEnums::GU_TRIGGER, origin);
On 2013/05/06 21:29:04, timsteele wrote:
> The real question we're asking is "What kind of GU is being requested here?".
It
> would flow more clearly if we ask that, rather than inferring it.
> 
> if (origin == sync_pb::SyncEnums::GU_TRIGGER) {
>   session->nudge_tracker()->FillProtoMessage(
>       ...;
> } else {
>   DCHECK(!session->nudge_tracker());
> }

Done.

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:387: void
SyncSchedulerImpl::ScheduleInvalidation(
On 2013/05/06 21:29:04, timsteele wrote:
> ScheduleInvalidationNudge seems better, to distinguish it from something that
> sounds like it might call directly into ticl.

Done.

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:685: } else if (mode_ == NORMAL_MODE &&
nudge_tracker_.SyncRequired()) {
On 2013/05/06 21:29:04, timsteele wrote:
> Tangential question, would NORMAL_MODE be better renamed to NUDGE_MODE?

I actually like the name NORMAL_MODE.  I think it does a good job of
communicating the most important property of that mode (that it's the "normal"
state of the scheduler), even if it doesn't explain what the mode does.  

NUDGE_MODE doesn't make a lot of sense unless you already understand what a
"nudge" is.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:14: size_t NudgeTracker::kMaxPayloadsPerType =
10;
On 2013/05/06 21:29:04, timsteele wrote:
> Might this be worth adding to client command as something we can override via
> cdd?

I don't see much benefit in fiddling with this knob.  

If the value is too small, then we risk unnecessarily asking the server to
perform a "full" sync.  If the value is too large, we risk almost nothing.  It
will cost us a few bytes of in-memory storage and network transmit.  I think we
should just set this to a large value and be done with it.  

That's the theory, anyway.  I'd like to start with this and see how it turns
out.  I'd be willing to add a client command if there is a need for it.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:104: SyncSourceInfo NudgeTracker::source_info()
const {
On 2013/05/06 21:29:04, timsteele wrote:
> This isn't such a trivial method.  GetSourceInfo() is a better name as per
style
> guide.

Done.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:114: ModelTypeSetToInvalidationMap(nudged_types,
"");
On 2013/05/06 21:29:04, timsteele wrote:
> prefer std::string() to "" for efficiency & explicitness (vs implicit
> constructor).

Done.

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

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.h:34: bool SyncRequired();
On 2013/05/06 21:29:04, timsteele wrote:
> nit - IsSyncRequired()

Done.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.h:52: void InvalidationsEnabled();
On 2013/05/06 21:29:04, timsteele wrote:
> Convention in Chrome for something like this would be to name it
> OnInvalidationsEnabled/Disabled.

Done.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/sync_session...
sync/sessions/sync_session.h:98: SyncSession(SyncSessionContext* context,
On 2013/05/06 21:29:04, timsteele wrote:
> Having factory methods with appropriate names that explain the purpose (e.g
> BuildSessionForNudge) would be preferable to overloading the constructor like
> this (for similar reasons to why the style guide forbids overloading
functions).
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.  I couldn't think of a good name for the non-nudge case, though. 
"BuildForNonNudge"?

[tim (not reviewing), 2013-05-07 20:34:13]

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:685: } else if (mode_ == NORMAL_MODE &&
nudge_tracker_.SyncRequired()) {
On 2013/05/07 18:45:39, rlarocque wrote:
> On 2013/05/06 21:29:04, timsteele wrote:
> > Tangential question, would NORMAL_MODE be better renamed to NUDGE_MODE?
> 
> I actually like the name NORMAL_MODE.  I think it does a good job of
> communicating the most important property of that mode (that it's the "normal"
> state of the scheduler), even if it doesn't explain what the mode does.  
> 
> NUDGE_MODE doesn't make a lot of sense unless you already understand what a
> "nudge" is.  

Fair, I like NORMAL_MODE as well.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:14: size_t NudgeTracker::kMaxPayloadsPerType =
10;
On 2013/05/07 18:45:39, rlarocque wrote:
> On 2013/05/06 21:29:04, timsteele wrote:
> > Might this be worth adding to client command as something we can override
via
> > cdd?
> 
> I don't see much benefit in fiddling with this knob.  
> 
> If the value is too small, then we risk unnecessarily asking the server to
> perform a "full" sync.  If the value is too large, we risk almost nothing.  It
> will cost us a few bytes of in-memory storage and network transmit.  I think
we
> should just set this to a large value and be done with it.  
> 
> That's the theory, anyway.  I'd like to start with this and see how it turns
> out.  I'd be willing to add a client command if there is a need for it.  

I agree with your logic, but the reason I ask is because we rarely ever know we
need to tweak it it until we need to tweak it, at which point it's useless to
add because of the sea of clients that won't understand the request anyway. 
Having a server switch might let us learn the impact of changing the value more
easily via experiment, would be more flexible to changes in the Tango trickles
design, and may come in handy firefighting a prod inicident. On the flip side,
there isn't a big downside, and it's easy to add since there's already a path to
plumb ClientCommand values through. So I encourage you to consider it once more
:p

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
sync/engine/sync_scheduler.h:97: virtual void ScheduleLocalRefreshRequest(
We should update the comment here to introduce the 'LocalRefreshRequest' and
'InvalidationNudge' concepts.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:811: if (notifications_enabled) {
nit - the majority of this file uses no { } on single line ifs. please try to
stick to that.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/syncer_unitte...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/syncer_unitte...
sync/engine/syncer_unittest.cc:183: // been tracking anything.
"... because they don't target scheduling logic and issue one-off sync
requests", or something to that effect.

https://codereview.chromium.org/14963002/diff/11001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/11001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:104: static SyncSession*
BuildForNonNudge(SyncSessionContext* context,
Build() and BuildForNudge() wouldn't be horrible, it helps distinguish the two
and won't need to change if we add a new Build routine or change BuildForNudge.

[rlarocque, 2013-05-07 21:50:57]

PTAL.

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/14963002/diff/2001/sync/sessions/nudge_tracke...
sync/sessions/nudge_tracker.cc:14: size_t NudgeTracker::kMaxPayloadsPerType =
10;
On 2013/05/07 20:34:13, timsteele wrote:
> On 2013/05/07 18:45:39, rlarocque wrote:
> > On 2013/05/06 21:29:04, timsteele wrote:
> > > Might this be worth adding to client command as something we can override
> via
> > > cdd?
> > 
> > I don't see much benefit in fiddling with this knob.  
> > 
> > If the value is too small, then we risk unnecessarily asking the server to
> > perform a "full" sync.  If the value is too large, we risk almost nothing. 
It
> > will cost us a few bytes of in-memory storage and network transmit.  I think
> we
> > should just set this to a large value and be done with it.  
> > 
> > That's the theory, anyway.  I'd like to start with this and see how it turns
> > out.  I'd be willing to add a client command if there is a need for it.  
> 
> I agree with your logic, but the reason I ask is because we rarely ever know
we
> need to tweak it it until we need to tweak it, at which point it's useless to
> add because of the sea of clients that won't understand the request anyway. 
> Having a server switch might let us learn the impact of changing the value
more
> easily via experiment, would be more flexible to changes in the Tango trickles
> design, and may come in handy firefighting a prod inicident. On the flip side,
> there isn't a big downside, and it's easy to add since there's already a path
to
> plumb ClientCommand values through. So I encourage you to consider it once
more
> :p

I still don't think it's necessary, but I added it.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
sync/engine/sync_scheduler.h:97: virtual void ScheduleLocalRefreshRequest(
On 2013/05/07 20:34:13, timsteele wrote:
> We should update the comment here to introduce the 'LocalRefreshRequest' and
> 'InvalidationNudge' concepts.

Done.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:811: if (notifications_enabled) {
On 2013/05/07 20:34:13, timsteele wrote:
> nit - the majority of this file uses no { } on single line ifs. please try to
> stick to that.

Does that apply to if / else pairs as well?  I don't recall seeing an else
without braces in Chromium code.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/syncer_unitte...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/syncer_unitte...
sync/engine/syncer_unittest.cc:183: // been tracking anything.
On 2013/05/07 20:34:13, timsteele wrote:
> "... because they don't target scheduling logic and issue one-off sync
> requests", or something to that effect.

Done.

https://codereview.chromium.org/14963002/diff/11001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/11001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:104: static SyncSession*
BuildForNonNudge(SyncSessionContext* context,
On 2013/05/07 20:34:13, timsteele wrote:
> Build() and BuildForNudge() wouldn't be horrible, it helps distinguish the two
> and won't need to change if we add a new Build routine or change
BuildForNudge.

Done.

[tim (not reviewing), 2013-05-07 22:40:33]

LGTM with final two comments addressed.

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/14963002/diff/11001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:811: if (notifications_enabled) {
On 2013/05/07 21:50:57, rlarocque wrote:
> On 2013/05/07 20:34:13, timsteele wrote:
> > nit - the majority of this file uses no { } on single line ifs. please try
to
> > stick to that.
> 
> Does that apply to if / else pairs as well?  I don't recall seeing an else
> without braces in Chromium code.  

It applies if all branches are single lines. If on the other hand, one branch of
the if-statement uses braces, then all must have braces.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:93: virtual void
OnReceivedClientInvalidationHintBufferSize(int size) = 0;
You use int here but size_t in NudgeTracker, was there a reason? I didn't see
any input validation that probably wouldn't hurt.

[rlarocque, 2013-05-07 22:56:49]

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:93: virtual void
OnReceivedClientInvalidationHintBufferSize(int size) = 0;
On 2013/05/07 22:40:34, timsteele wrote:
> You use int here but size_t in NudgeTracker, was there a reason? I didn't see
> any input validation that probably wouldn't hurt.

The protobufs provide us with ints, so I've used ints in the functions that are
meant to transmit data from the protobufs to other parts of the code.  

All The SyncSession::Delegate implementations will treat this as an int, because
that's the format of the message.  

The NudgeTracker deals in size_t, because that's the type of the size of a
vector (or any in-memory list).  The value passed to
NudgeTracker::SetHintBufferSize() will only be used in comparisons against a
size_t, so we'd might as well ask for it and store it in that format.  

What sort of input checking were you looking for?  Should we make sure the int
is non-negative before passing it along to the nudge tracker?  

Should we check that it's within some reasonable bounds?  It seems like that
would defeat the point of making it server-controlled.

[tim (not reviewing), 2013-05-07 23:35:50]

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/14963002/diff/20001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:93: virtual void
OnReceivedClientInvalidationHintBufferSize(int size) = 0;
On 2013/05/07 22:56:49, rlarocque wrote:
> On 2013/05/07 22:40:34, timsteele wrote:
> > You use int here but size_t in NudgeTracker, was there a reason? I didn't
see
> > any input validation that probably wouldn't hurt.
> 
> The protobufs provide us with ints, so I've used ints in the functions that
are
> meant to transmit data from the protobufs to other parts of the code.  
> 
> All The SyncSession::Delegate implementations will treat this as an int,
because
> that's the format of the message.  
> 
> The NudgeTracker deals in size_t, because that's the type of the size of a
> vector (or any in-memory list).  The value passed to
> NudgeTracker::SetHintBufferSize() will only be used in comparisons against a
> size_t, so we'd might as well ask for it and store it in that format.  
> 
> What sort of input checking were you looking for?  Should we make sure the int
> is non-negative before passing it along to the nudge tracker?  
> 
> Should we check that it's within some reasonable bounds?  It seems like that
> would defeat the point of making it server-controlled.

Sanity checking that the value was greater than zero was what I had in mind. I
left it open in case you had any other ideas as well.

[commit-bot: I haz the power, 2013-05-08 22:51:14]

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

[commit-bot: I haz the power, 2013-05-09 05:51:06]

Change committed as 199136