[Sync] Parse and save per-data type throttle data

Parse and Store the throttled datatypes and the unthrottling time, which is obtained from the server response, in the sync_session_context. 

This is the first patch for per datatype throttling feature. The next patch would consume this data to decide what items to commit.

BUG=104819
TEST=sync_unit_tests.exe, sync_integration_tests.exe, unit_tests.exe


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

[lipalani1, 2011-11-18 23:04:45]

Whoever has the time please do the review. 
Since Fred seems out, Tim I think you get the first dibs!

[akalin, 2011-11-21 20:04:26]

Hmm it's hard to get a good sense of this CL because only bits and pieces of the
plumbing is there (although I understand that you're trying to split up the
CLs).

I think a more logical sequence of CLs would be:

1) A CL with the protocol changes and plumbing to fill in the ModelTypeSet in
sync_protocol_error.h (so the changes to sync.proto and sync_protocol_error.h,
plus some other ones)
2) Plumbing up from the sync_protocol_error.h to fill in the sync context (i.e.,
the remaining changes in this CL, plus some other ones, e.g. what actually calls
HandleThrottling)

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:195: DCHECK(error.error_type ==
browser_sync::THROTTLED);
DCHECK_EQ

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (left):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:11: #include
"base/gtest_prod_util.h"
#include "base/time.h"

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:13: #include
"chrome/browser/sync/sessions/sync_session_context.h"
forward-declare context and syncprotocolerror

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:30: class SyncSession;
remove this since you're #including now

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:120: static void
SyncerProtoUtil::HandleThrottling(
remove SyncerProtoUtil::

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:630: // a server.
add a comment about how migrated_data_type_id is used (for legacy reasons)
instead of new error_data_type_ids field in Error

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:655: // then the whole client(all
datatypes) is throttled.
space before (

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:656: repeated int32
throttled_data_type_ids = 5;
i think this should be named more generically, like error_data_type_ids, and the
comment moved to TRANSIENT_ERROR above

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.cc (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.cc:53: const base::TimeTicks&
time) {
indent

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.cc:57:
DCHECK(unthrottle_times_.count(*it) == 0);
DCHECK_EQ

[lipalani1, 2011-11-21 21:55:38]

Fixed the feedback.

About the CL splitup:
The current split up is:
CL1: Handle the communication from server to client of what types are
throttled.(i.e., parsing the data and storing it)
CL2: Handle the communication from client to server on how the throttled types
are not sent as part of the sync cycle.(i.e., consuming the saved data).

Had I written the above in the description would it have been easier for you to
follow? if you dont think so I can split up the CL to:
1. parsing.
2. saving and consuming.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:195: DCHECK(error.error_type ==
browser_sync::THROTTLED);
On 2011/11/21 20:04:26, akalin wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (left):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:11: #include
"base/gtest_prod_util.h"
On 2011/11/21 20:04:26, akalin wrote:
> #include "base/time.h"

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:13: #include
"chrome/browser/sync/sessions/sync_session_context.h"
On 2011/11/21 20:04:26, akalin wrote:
> forward-declare context and syncprotocolerror

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:30: class SyncSession;
On 2011/11/21 20:04:26, akalin wrote:
> remove this since you're #including now

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:120: static void
SyncerProtoUtil::HandleThrottling(
On 2011/11/21 20:04:26, akalin wrote:
> remove SyncerProtoUtil::

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:630: // a server.
On 2011/11/21 20:04:26, akalin wrote:
> add a comment about how migrated_data_type_id is used (for legacy reasons)
> instead of new error_data_type_ids field in Error

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:655: // then the whole client(all
datatypes) is throttled.
Not sure I understand?
On 2011/11/21 20:04:26, akalin wrote:
> space before (

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:656: repeated int32
throttled_data_type_ids = 5;
Renamed. Not sure I understand the rest of the comment.
On 2011/11/21 20:04:26, akalin wrote:
> i think this should be named more generically, like error_data_type_ids, and
the
> comment moved to TRANSIENT_ERROR above

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.cc (right):

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.cc:53: const base::TimeTicks&
time) {
On 2011/11/21 20:04:26, akalin wrote:
> indent

Done.

http://codereview.chromium.org/8595023/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.cc:57:
DCHECK(unthrottle_times_.count(*it) == 0);
Actually removed it. I thought about this further and it seems if the server
throttles a new type we should not have the restriction for it to send down only
the new types.
On 2011/11/21 20:04:26, akalin wrote:
> DCHECK_EQ

[akalin, 2011-11-21 22:21:17]

On Mon, Nov 21, 2011 at 1:55 PM,  <lipalani@chromium.org> wrote:
> Fixed the feedback.
>
> About the CL splitup:
> The current split up is:
> CL1: Handle the communication from server to client of what types are
> throttled.(i.e., parsing the data and storing it)
> CL2: Handle the communication from client to server on how the throttled
> types
> are not sent as part of the sync cycle.(i.e., consuming the saved data).
>
> Had I written the above in the description would it have been easier for you
> to
> follow? if you dont think so I can split up the CL to:
> 1. parsing.
> 2. saving and consuming.

I don't think this CL actually implements CL1 as you describe above.
For example, you add the function HandleThrottling, but I don't see it
being called anywhere aside from test code.

[akalin, 2011-11-21 22:21:44]

On Mon, Nov 21, 2011 at 2:21 PM, Fred Akalin <akalin@chromium.org> wrote:
> On Mon, Nov 21, 2011 at 1:55 PM,  <lipalani@chromium.org> wrote:
>> Fixed the feedback.
>>
>> About the CL splitup:
>> The current split up is:
>> CL1: Handle the communication from server to client of what types are
>> throttled.(i.e., parsing the data and storing it)
>> CL2: Handle the communication from client to server on how the throttled
>> types
>> are not sent as part of the sync cycle.(i.e., consuming the saved data).
>>
>> Had I written the above in the description would it have been easier for you
>> to
>> follow? if you dont think so I can split up the CL to:
>> 1. parsing.
>> 2. saving and consuming.
>
> I don't think this CL actually implements CL1 as you describe above.
> For example, you add the function HandleThrottling, but I don't see it
> being called anywhere aside from test code.
>

Oh, now I see it.  My mistake!

[akalin, 2011-11-21 22:36:04]

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:199:
delegate->OnSilencedUntil(throttled_until);
add comment saying that no data types means we're completely throttled

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:276: DCHECK(error.error_type()
== ClientToServerResponse::THROTTLED);
DCHECK_EQ

Also add comment saying that THROTTLED is currently the only error code that
uses error_data_type_ids.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:30: class SyncerProtoUtil;
don't you mean SyncProtocolError?

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:121: static void
HandleThrottling(const SyncProtocolError& error,
I think HandleThrottleError is a better name

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:630: // a server. The migrated datatypes
would be
would be present -> are

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:656: // is absent then the whole
client(all datatypes) is throttled.
space before '('

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:50: typedef
std::map<syncable::ModelType, base::TimeTicks> UnthrottleTimes;
i think this can be private to SyncSessionContext

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
comment that it's virtual for unit tests

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
add a TODO to actually use the unthrottle times (for your next patch)

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
I think SetUnthrottleTime is a better name

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:160: UnthrottleTimes
unthrottle_times_;
add comment saying that this is a map from throttled data types to the time at
which they can be unthrottled.

[akalin, 2011-11-21 22:38:40]

> Fixed the feedback.
> 
> About the CL splitup:
> The current split up is:
> CL1: Handle the communication from server to client of what types are
> throttled.(i.e., parsing the data and storing it)
> CL2: Handle the communication from client to server on how the throttled types
> are not sent as part of the sync cycle.(i.e., consuming the saved data).
> 
> Had I written the above in the description would it have been easier for you
to
> follow? if you dont think so I can split up the CL to:
> 1. parsing.
> 2. saving and consuming.

Actually, it was my mistake that led me to think your CL was incomplete.  Sorry!

I think the CL subject should be descriptive of the actual change, say:

"[Sync] Parse and save per-data type throttle data"

and then the part about this being the first patch should be in the description.
 That way the subject is enough to summarize the change (for people not familiar
with sync).

[akalin, 2011-11-21 22:39:05]

> Actually, it was my mistake that led me to think your CL was incomplete. 
Sorry!

Just to clarify, I think your CL splitup is fine as is.

[lipalani1, 2011-11-22 00:47:35]

Please review. All feedback fixed.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:199:
delegate->OnSilencedUntil(throttled_until);
On 2011/11/21 22:36:04, akalin wrote:
> add comment saying that no data types means we're completely throttled

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:276: DCHECK(error.error_type()
== ClientToServerResponse::THROTTLED);
On 2011/11/21 22:36:04, akalin wrote:
> DCHECK_EQ
> 
> Also add comment saying that THROTTLED is currently the only error code that
> uses error_data_type_ids.

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:30: class SyncerProtoUtil;
ah I think the file has been added by some other file. Otherwise I would have
gotten a error. Fixed now.
On 2011/11/21 22:36:04, akalin wrote:
> don't you mean SyncProtocolError?

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:121: static void
HandleThrottling(const SyncProtocolError& error,
On 2011/11/21 22:36:04, akalin wrote:
> I think HandleThrottleError is a better name

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:630: // a server. The migrated datatypes
would be
On 2011/11/21 22:36:04, akalin wrote:
> would be present -> are

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:656: // is absent then the whole
client(all datatypes) is throttled.
On 2011/11/21 22:36:04, akalin wrote:
> space before '('

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:50: typedef
std::map<syncable::ModelType, base::TimeTicks> UnthrottleTimes;
On 2011/11/21 22:36:04, akalin wrote:
> i think this can be private to SyncSessionContext

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
On 2011/11/21 22:36:04, akalin wrote:
> comment that it's virtual for unit tests

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
On 2011/11/21 22:36:04, akalin wrote:
> add a TODO to actually use the unthrottle times (for your next patch)

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:113: virtual void
AddUnthrottleTime(const syncable::ModelTypeSet& types,
On 2011/11/21 22:36:04, akalin wrote:
> I think SetUnthrottleTime is a better name

Done.

http://codereview.chromium.org/8595023/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:160: UnthrottleTimes
unthrottle_times_;
On 2011/11/21 22:36:04, akalin wrote:
> add comment saying that this is a map from throttled data types to the time at
> which they can be unthrottled.

Done.

[akalin, 2011-11-22 04:26:33]

LGTM

http://codereview.chromium.org/8595023/diff/15011/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/8595023/diff/15011/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/sync.proto:655: // Currently only meaningful if the
|error_type| is throttled. If this field
"the |error_type| is throttled" -> "|error_type| is THROTTLED"

http://codereview.chromium.org/8595023/diff/15011/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/8595023/diff/15011/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_context.h:111: // This is virtual to
test this in unit tests.
"to test this in" -> "for"

http://codereview.chromium.org/8595023/diff/15011/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_context.h:163: // This is a map from
throttled datatypes to the time at which they can be
datatypes -> data types

[commit-bot: I haz the power, 2011-11-22 23:31:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8595023/24001

[commit-bot: I haz the power, 2011-11-23 01:30:58]

Try job failure for 8595023-24001 (retry) on mac_rel for steps "browser_tests,
ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-23 18:57:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8595023/24001

[commit-bot: I haz the power, 2011-11-23 20:46:02]

Change committed as 111408