Raise the throttling limits of the storage API. (issue 702303002 by kalman@chromium.org)

[kal...@chromium.org]

Reviewers: Nicolas Zea,

Message:
Please sanity check, I picked all of these numbers semi-arbitrarily.

Description:
Raise the throttling limits of the storage API.

The per-hour limit is now 3600, allowing 1 write operation per second over
an
extended period of time. To provide protection against short-term flooding,
add
a new per-minute limit of 300 (5 per second).

BUG=406406
R=z...@chromium.org

Please review this at https://codereview.chromium.org/702303002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+17, -9 lines):
M extensions/browser/api/storage/storage_api.cc
M extensions/common/api/storage.json


Index: extensions/browser/api/storage/storage_api.cc
diff --git a/extensions/browser/api/storage/storage_api.cc
b/extensions/browser/api/storage/storage_api.cc
index
5e984bc8f95362c6034de5f8bd265264cf2c79c1..38c8e7181ac47b48fb7146eb5d3fb3a441aba76a
100644
--- a/extensions/browser/api/storage/storage_api.cc
+++ b/extensions/browser/api/storage/storage_api.cc
@@ -149,14 +149,18 @@ std::vector<std::string> GetKeys(const
base::DictionaryValue& dict) {

// Creates quota heuristics for settings modification.
void GetModificationQuotaLimitHeuristics(QuotaLimitHeuristics* heuristics)
{
- QuotaLimitHeuristic::Config longLimitConfig = {
- // See storage.json for current value.
- core_api::storage::sync::MAX_WRITE_OPERATIONS_PER_HOUR,
- base::TimeDelta::FromHours(1)
- };
+ // See storage.json for the current value of these limits.
+ QuotaLimitHeuristic::Config short_limit_config = {
+ core_api::storage::sync::MAX_WRITE_OPERATIONS_PER_MINUTE,
+ base::TimeDelta::FromMinutes(1)};
+ QuotaLimitHeuristic::Config long_limit_config = {
+ core_api::storage::sync::MAX_WRITE_OPERATIONS_PER_HOUR,
+ base::TimeDelta::FromHours(1)};
+ heuristics->push_back(new QuotaService::TimedLimit(
+ short_limit_config, new QuotaLimitHeuristic::SingletonBucketMapper(),
+ "MAX_WRITE_OPERATIONS_PER_MINUTE"));
heuristics->push_back(new QuotaService::TimedLimit(
- longLimitConfig,
- new QuotaLimitHeuristic::SingletonBucketMapper(),
+ long_limit_config, new QuotaLimitHeuristic::SingletonBucketMapper(),
"MAX_WRITE_OPERATIONS_PER_HOUR"));
};

Index: extensions/common/api/storage.json
diff --git a/extensions/common/api/storage.json
b/extensions/common/api/storage.json
index
34e789c349a90e3d7ef058dc05854bbb82451cf6..14ecc6c62c3ba96bc5be98de35e6ea87d2c96e2a
100644
--- a/extensions/common/api/storage.json
+++ b/extensions/common/api/storage.json
@@ -189,8 +189,12 @@
"description": "The maximum number of items that can be stored
in sync storage. Updates that would cause this limit to be exceeded will
fail immediately and set $(ref:runtime.lastError)."
},
"MAX_WRITE_OPERATIONS_PER_HOUR": {
- "value": 1000,
- "description": "The maximum number of <code>set</code>,
<code>remove</code>, or <code>clear</code> operations that can be performed
each hour. Updates that would cause this limit to be exceeded fail
immediately and set $(ref:runtime.lastError)."
+ "value": 3600,
+ "description": "<p>The maximum number of <code>set</code>,
<code>remove</code>, or <code>clear</code> operations that can be performed
each hour. This is 1 per second, a lower ceiling than the short term higher
writes-per-minute limit.</p><p>Updates that would cause this limit to be
exceeded fail immediately and set $(ref:runtime.lastError).</p>"
+ },
+ "MAX_WRITE_OPERATIONS_PER_MINUTE": {
+ "value": 300,
+ "description": "<p>The maximum number of <code>set</code>,
<code>remove</code>, or <code>clear</code> operations that can be performed
each minute. This is 5 per second, providing higher throughput than
writes-per-hour over a shorter period of time.</p><p>Updates that would
cause this limit to be exceeded fail immediately and set
$(ref:runtime.lastError).</p>"
},
"MAX_SUSTAINED_WRITE_OPERATIONS_PER_MINUTE": {
"value": 1000000,

[z...@chromium.org]

I kind of think the limits should be reversed. That we want a more
conservative
per-hour limit to discourage abuse, and a more generous per-minute limit, to
allow bursts.

Given that throttling isn't a permanent state anymore, I'd argue 1 per
second
for the per-minute limit (60 per minute), and maybe 1 every 10 seconds for
the
hour limit (360 per hour).

https://codereview.chromium.org/702303002/

[kal...@chromium.org]

On 2014/11/06 00:23:53, Nicolas Zea wrote:
> I kind of think the limits should be reversed. That we want a more
conservative
> per-hour limit to discourage abuse, and a more generous per-minute limit,
> to
> allow bursts.

I think that's what I've written? Else I'm reading numbers wrong and/or
misunderstanding you.

> Given that throttling isn't a permanent state anymore, I'd argue 1 per
> second
> for the per-minute limit (60 per minute), and maybe 1 every 10 seconds
> for the
> hour limit (360 per hour).

I think 1 per second is a bit strict, at least 2 per second would be
reasonable
(e.g. flushing a cache once per second should work). 360 is doable, I guess
I'd
prefer it to be more lenient - and to add metrics for this actually - but
that
involves diving into the quota service infrastructure again :\

https://codereview.chromium.org/702303002/

[z...@chromium.org]

Oh oops, I misread your commit statement. You're right, the way you had it
was
correct. And 2 per second in a minute sounds good as well. Maybe 1800 an
hour
then? (Every 2 seconds)

https://codereview.chromium.org/702303002/

[kal...@chromium.org]

https://codereview.chromium.org/702303002/

[kal...@chromium.org]

Err, or should I say, done.

https://codereview.chromium.org/702303002/

[z...@chromium.org]

lgtm



https://codereview.chromium.org/702303002/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/702303002/20001


https://codereview.chromium.org/702303002/

[commi...@chromium.org]

Exceeded time limit waiting for builds to trigger.

https://codereview.chromium.org/702303002/

[commi...@chromium.org]

[commi...@chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/702303002/

[commi...@chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/f7a2e0c4baa4c8029a87b0264abfae8b16865dac
Cr-Commit-Position: refs/heads/master@{#303248}

https://codereview.chromium.org/702303002/