Make the throttle delay configurable by the server.

Make the throttle delay configurable by the server.

BUG=
TEST=


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

[lipalani1, 2011-08-10 00:46:04]

Please review.

Note: I am writing a full set of integration test cases for error handling.
Throttling is tested as part of that CL. (Throttling test cases are under CR.
Submitting this patch will unblock it)

[tim (not reviewing), 2011-08-10 00:50:48]

+Fred, who is working on throttling code

[lipalani, 2011-08-10 00:57:44]

ha..
I did not know Fred was working on something similar.
Fred - let me know if this needs to go in or you have a patch that addresses
this.
Thanks!
regards,
Lingesh

On Tue, Aug 9, 2011 at 5:50 PM, <tim@chromium.org> wrote:

> +Fred, who is working on throttling code
>
>
>
http://codereview.chromium.**org/7605021/<http://codereview.chromium.org/7605...
>

[akalin, 2011-08-10 01:24:36]

I think this is sufficiently different from what I'm working on (adjusting nudge
delay for sessions).  It's probably a more permanent solution, anyway.

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:192: return
command.throttle_delay();
use TimeDelta::FromSeconds()

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:194: return
kSyncDelayAfterThrottled;
here, too

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:118: static int
GetThrottleDelay(const sync_pb::ClientToServerResponse& response);
change to return TimeDelta

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:584: optional int32 throttle_delay = 4;
// in seconds
You probably want to re-sync and use a different number (i just checked in a
change that uses #4)

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:584: optional int32 throttle_delay = 4;
// in seconds
rename to throttle_delay_seconds

[tim (not reviewing), 2011-08-10 01:27:45]

I was referring to the per-data-type throttling code, which involves
server-controlled logic.

On Tue, Aug 9, 2011 at 6:24 PM, <akalin@chromium.org> wrote:

> I think this is sufficiently different from what I'm working on (adjusting
> nudge
> delay for sessions).  It's probably a more permanent solution, anyway.
>
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/engine/syncer_**proto_util.cc<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/syncer_proto_util.cc>
> File chrome/browser/sync/engine/**syncer_proto_util.cc (right):
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/engine/syncer_**proto_util.cc#newcode192<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/syncer_proto_util.cc#newcode192>
> chrome/browser/sync/engine/**syncer_proto_util.cc:192: return
> command.throttle_delay();
> use TimeDelta::FromSeconds()
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/engine/syncer_**proto_util.cc#newcode194<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/syncer_proto_util.cc#newcode194>
> chrome/browser/sync/engine/**syncer_proto_util.cc:194: return
> kSyncDelayAfterThrottled;
> here, too
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/engine/syncer_**proto_util.h<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/syncer_proto_util.h>
> File chrome/browser/sync/engine/**syncer_proto_util.h (right):
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/engine/syncer_**proto_util.h#newcode118<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/syncer_proto_util.h#newcode118>
> chrome/browser/sync/engine/**syncer_proto_util.h:118: static int
>
> GetThrottleDelay(const sync_pb::**ClientToServerResponse& response);
> change to return TimeDelta
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/protocol/sync.**proto<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol/sync.proto>
> File chrome/browser/sync/protocol/**sync.proto (right):
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/protocol/sync.**proto#newcode584<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol/sync.proto#newcode584>
> chrome/browser/sync/protocol/**sync.proto:584: optional int32
>
> throttle_delay = 4; // in seconds
> You probably want to re-sync and use a different number (i just checked
> in a change that uses #4)
>
> http://codereview.chromium.**org/7605021/diff/5001/chrome/**
>
browser/sync/protocol/sync.**proto#newcode584<http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol/sync.proto#newcode584>
> chrome/browser/sync/protocol/**sync.proto:584: optional int32
>
> throttle_delay = 4; // in seconds
> rename to throttle_delay_seconds
>
>
>
http://codereview.chromium.**org/7605021/<http://codereview.chromium.org/7605...
>

[akalin, 2011-08-10 01:40:11]

On 2011/08/10 01:27:45, timsteele wrote:
> I was referring to the per-data-type throttling code, which involves
> server-controlled logic.

Oh, right.  Well I still have to figure out a design for that.  But this looks
okay for now; we can always expand it later.

[lipalani1, 2011-08-10 19:04:05]

Please review.

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:192: return
command.throttle_delay();
On 2011/08/10 01:24:36, akalin wrote:
> use TimeDelta::FromSeconds()

Done.

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:194: return
kSyncDelayAfterThrottled;
On 2011/08/10 01:24:36, akalin wrote:
> here, too

Done.

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.h:118: static int
GetThrottleDelay(const sync_pb::ClientToServerResponse& response);
On 2011/08/10 01:24:36, akalin wrote:
> change to return TimeDelta

Done.

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

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:584: optional int32 throttle_delay = 4;
// in seconds
On 2011/08/10 01:24:36, akalin wrote:
> You probably want to re-sync and use a different number (i just checked in a
> change that uses #4)

Done.

http://codereview.chromium.org/7605021/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/sync.proto:584: optional int32 throttle_delay = 4;
// in seconds
On 2011/08/10 01:24:36, akalin wrote:
> rename to throttle_delay_seconds

Done.

[akalin, 2011-08-10 19:48:14]

http://codereview.chromium.org/7605021/diff/10001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/7605021/diff/10001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_proto_util.cc:194: return
base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
i don't like repeating the constant twice.  How about using:

base::TimeDelta throttle_delay =
    base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
if (response.has_client_command()) {
  const ...;
  if (command.has_throttle_delay_seconds()) {
    throttle_delay = base...;
  }
}
return throttle_delay;

or

base::TimeDelta default_throttle_delay =
    base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
if (!response.has_client_command()) {
  return default_throttle_delay;
}
const ...;
if (!command.has_throttle_delay_seconds()) {
  return default_throttle_delay;
}
return FromSeconds(command.throttle_delay_seconds());

[lipalani1, 2011-08-10 20:38:57]

Please review.

http://codereview.chromium.org/7605021/diff/10001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/7605021/diff/10001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_proto_util.cc:194: return
base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
On 2011/08/10 19:48:14, akalin wrote:
> i don't like repeating the constant twice.  How about using:
> 
> base::TimeDelta throttle_delay =
>     base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
> if (response.has_client_command()) {
>   const ...;
>   if (command.has_throttle_delay_seconds()) {
>     throttle_delay = base...;
>   }
> }
> return throttle_delay;
> 
> or
> 
> base::TimeDelta default_throttle_delay =
>     base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled);
> if (!response.has_client_command()) {
>   return default_throttle_delay;
> }
> const ...;
> if (!command.has_throttle_delay_seconds()) {
>   return default_throttle_delay;
> }
> return FromSeconds(command.throttle_delay_seconds());

Done.

[akalin, 2011-08-11 00:02:08]

LGTM

[commit-bot: I haz the power, 2011-08-11 02:52:04]

Change committed as 96312