Add quota limits for push command #545

ikhoon · 2020-11-20T12:55:13Z

Motivation:

Central Dogma is a read centric application which means
heavy writes could be a risk to the reliabilty of the system.
By limiting write requests, Central Dogma is able to respond flexibly to burst traffic.

Modifications:

Add QuotaConfig to CentralDogmaConfig for a global configuration. e.g.

"writeQuotaPerRepository": {
  "requestQuota" : 5,
  "timeWindowSeconds": 1
}

Add QuotaConfig to RepositoryMetadata for a repoistory level configration
Use Guava's RateLimiter to limit requests for a standalone mode
Use InterProcessSemaphoreV2 to limit requests for a replication mode
Add a write qouta REST API to MetadataService
- PATCH /metadata/{projectName}/repos/{repoName}/quota/write
Refactor test code to build Zookeeper cluster easily

Result:

You can now set API quotas to push requests for repositories.
Fixes Limits and quotas on API requests #528

Motivation: Central Dogma is a read centric application which means heavy writes could be a risk to the reliabilty of the system. By limiting write requests, Central Dogma is able to respond flexibly to burst traffic. Modifications: - Add `QuotaConfig` to `CentralDogmaConfig` for a global configration ```json "writeQuotaPerRepository": { "requestQuota" : 10, "timeWindowSeconds": 1 } ``` - Add `QuotaConfig` to `RepositoryMetadata` for a repoistory level configration - Use Guava's `RateLimiter` to limit requests for a standalone mode - Use `InterProcessSemaphoreV2` to limit requests for a replication mode - Add a write qouta REST API to MetadataService - PATCH `/metadata/{projectName}/repos/{repoName}/quota/write` - Refactor test code to build Zookeeper cluster easily Result: - You can now set API quotas to push requests for repositories. - Fixes line#528

minwoox

Nice work!

common/src/main/java/com/linecorp/centraldogma/common/TooManyRequestsException.java

it/src/test/java/com/linecorp/centraldogma/it/ReplicationWriteQuotaTest.java

...ain/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

site/src/sphinx/setup-configuration.rst

server/src/test/java/com/linecorp/centraldogma/server/internal/replication/Replica.java

minwoox · 2020-12-03T03:18:25Z

Could you also check the test failure?
https://travis-ci.com/github/line/centraldogma/builds/203727256#L912

ikhoon · 2020-12-03T03:20:06Z

Could you also check the test failure?
travis-ci.com/github/line/centraldogma/builds/203727256#L912

Oops...

codecov · 2020-12-04T02:05:32Z

Codecov Report

Merging #545 (5176fa4) into master (3927148) will increase coverage by 0.37%.
The diff coverage is 76.78%.

@@             Coverage Diff              @@
##             master     #545      +/-   ##
============================================
+ Coverage     69.75%   70.13%   +0.37%     
- Complexity     3187     3267      +80     
============================================
  Files           325      328       +3     
  Lines         12796    13007     +211     
  Branches       1376     1402      +26     
============================================
+ Hits           8926     9122     +196     
- Misses         3002     3010       +8     
- Partials        868      875       +7

Impacted Files	Coverage Δ	Complexity Δ
...ogma/server/command/ForwardingCommandExecutor.java	`46.66% <0.00%> (-7.18%)`	`5.00 <0.00> (ø)`
...er/internal/admin/auth/SessionTokenAuthorizer.java	`100.00% <ø> (+5.55%)`	`6.00 <0.00> (ø)`
.../centraldogma/common/TooManyRequestsException.java	`40.00% <40.00%> (ø)`	`3.00 <3.00> (?)`
.../com/linecorp/centraldogma/server/QuotaConfig.java	`40.00% <40.00%> (ø)`	`5.00 <5.00> (?)`
...ecorp/centraldogma/server/CentralDogmaBuilder.java	`53.96% <66.66%> (+3.96%)`	`23.00 <1.00> (+4.00)`
...ntraldogma/server/metadata/RepositoryMetadata.java	`65.51% <66.66%> (-2.49%)`	`10.00 <3.00> (+1.00)`	⬇️
...rver/internal/replication/SettableSharedCount.java	`73.68% <73.68%> (ø)`	`6.00 <6.00> (?)`
...internal/replication/ZooKeeperCommandExecutor.java	`77.73% <82.02%> (+0.72%)`	`90.00 <24.00> (+20.00)`
...ogma/server/command/StandaloneCommandExecutor.java	`86.95% <90.00%> (+1.62%)`	`60.00 <24.00> (+23.00)`
...com/linecorp/centraldogma/server/CentralDogma.java	`76.20% <100.00%> (+0.72%)`	`47.00 <0.00> (+1.00)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3927148...5176fa4. Read the comment docs.

minwoox

Just some nits. 😄

minwoox · 2020-12-07T01:23:54Z

server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java

+            final double permitsPerSecond =
+                    permitsForRepo != 0 ? permitsForRepo : this.permitsPerSecond;
+            if (permitsPerSecond > 0) {
+                return writeRateLimiters.compute(projectName + '/' + repoName, (key, rateLimiter) -> {


How about adding todo for removing the rate limiter when the repository is removed?

I can address it in this PR. :-)

...ain/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

minwoox · 2020-12-07T01:35:20Z

...ain/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

+            count.setCount(requestQuota);
+        }
+        final InterProcessSemaphoreV2 semaphore = semaphoreAndCount.getKey();
+        final Lease lease = semaphore.acquire(200, TimeUnit.MILLISECONDS);


the average QPS was 7~8 per repository.
This QPS test was executed including this lease mechanism, right?
Then, 200ms will be fine I guess. 😄

minwoox · 2020-12-07T01:38:10Z

site/src/sphinx/setup-configuration.rst

+-  ``writeQuotaPerRepository``
+
+  - the maximum allowed write quota per repository. If ``requestQuota`` is set to 10 and
+    ``timeWindowSeconds`` is set to 1, :ref:`pushing-a-commit`` cannot exceeds 10 QPS; if exceeded,


Suggested change

``timeWindowSeconds`` is set to 1, :ref:`pushing-a-commit`` cannot exceeds 10 QPS; if exceeded,

``timeWindowSeconds`` is set to 1, :ref:`pushing-a-commit` cannot exceed 10 QPS; if exceeded,

ikhoon · 2020-12-07T07:36:39Z

the average QPS was 7~8 per repository.
This QPS test was executed including this lease mechanism, right?
Then, 200ms will be fine I guess. 😄

Yes. The benchmark code included quota limitation.

minwoox · 2020-12-07T11:02:03Z

https://travis-ci.com/github/line/centraldogma/builds/206964876#L716
Is this related? 🤔

ikhoon · 2020-12-07T11:04:16Z

travis-ci.com/github/line/centraldogma/builds/206964876#L716
Is this related? 🤔

Hmm... I'm not sure, let me check it more.

minwoox

Great job, @ikhoon! Thanks a lot for working on this. 😄
BTW, how about adding a metric to get to know how frequently TooManyRequestException happens?

minwoox · 2020-12-08T05:42:31Z

server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java

+    @Override
+    public final void setWriteQuota(String projectName, String repoName, @Nullable QuotaConfig writeQuota) {
+        final double permitsForRepo = writeQuota != null ? writeQuota.permitsPerSecond() : 0;
+        final double permitsPerSecond = permitsForRepo != 0 ? permitsForRepo : this.permitsPerSecond;


What happen if this.permitsPerSecond is -1?

If this.permitsPerSecond is -1, this method should not be called.
This method is called by MetadataService.updateWriteQuota() and StandaloneCommandExecutor.getRateLimiter(). They are checked already.

Anyway, it would better return with no-op, if this.permitsPerSecond is -1.

ikhoon · 2020-12-08T07:21:58Z

BTW, how about adding a metric to get to know how frequently TooManyRequestException happens?

Can't we use Armeria's request metrics with status?

ikhoon · 2020-12-08T07:26:43Z

Great job, @ikhoon! Thanks a lot for working on this. 😄

Thanks for reviewing! 🙇‍♂️

minwoox · 2020-12-08T07:59:05Z

https://travis-ci.com/github/line/centraldogma/builds/207208682#L703
Could you check this NPE?

ikhoon · 2020-12-09T07:58:58Z

travis-ci.com/github/line/centraldogma/builds/207208682#L703
Could you check this NPE?

Oops... I missed this message. 😅

minwoox · 2020-12-10T02:11:47Z

🎉 🎉 🎉

ikhoon · 2020-12-10T02:12:13Z

Thanks for reviewing!

ikhoon added the new feature label Nov 20, 2020

ikhoon added this to the 0.46.1 milestone Nov 20, 2020

ikhoon requested review from minwoox and trustin as code owners November 20, 2020 12:55

Fix broken tests

3a35e06

ikhoon modified the milestones: 0.46.1, 0.47.0 Dec 2, 2020

minwoox reviewed Dec 3, 2020

View reviewed changes

ikhoon added 2 commits December 3, 2020 16:50

Address comments by @minwoox

c717b52

Add null check

3a98200

Revert writeQuotaPerRepository and set 5 to requestQuota

1bed846

minwoox reviewed Dec 7, 2020

View reviewed changes

Address comments by @minwoox

8baf708

ikhoon added 2 commits December 7, 2020 21:09

Update quota when changed

170a9e0

Invalidate cache when repo is removed

d1373f5

minwoox approved these changes Dec 8, 2020

View reviewed changes

Address comments by @minwoox

ac7ad8e

Set null as a default quota

9ab6835

Close cluster synchronously

5176fa4

minwoox merged commit 082d87b into line:master Dec 10, 2020

ikhoon deleted the quota branch December 10, 2020 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quota limits for push command #545

Add quota limits for push command #545

ikhoon commented Nov 20, 2020 •

edited

Loading

minwoox left a comment

minwoox commented Dec 3, 2020

ikhoon commented Dec 3, 2020

codecov bot commented Dec 4, 2020 •

edited

Loading

minwoox left a comment

minwoox Dec 7, 2020

ikhoon Dec 7, 2020

minwoox Dec 7, 2020

minwoox Dec 7, 2020

ikhoon commented Dec 7, 2020 •

edited

Loading

minwoox commented Dec 7, 2020

ikhoon commented Dec 7, 2020

minwoox left a comment •

edited

Loading

minwoox Dec 8, 2020

ikhoon Dec 8, 2020 •

edited

Loading

ikhoon commented Dec 8, 2020

ikhoon commented Dec 8, 2020

minwoox commented Dec 8, 2020

ikhoon commented Dec 9, 2020

minwoox commented Dec 10, 2020

ikhoon commented Dec 10, 2020

	``timeWindowSeconds`` is set to 1, :ref:`pushing-a-commit`` cannot exceeds 10 QPS; if exceeded,
	``timeWindowSeconds`` is set to 1, :ref:`pushing-a-commit` cannot exceed 10 QPS; if exceeded,

Add quota limits for push command #545

Add quota limits for push command #545

Conversation

ikhoon commented Nov 20, 2020 • edited Loading

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented Dec 3, 2020

ikhoon commented Dec 3, 2020

codecov bot commented Dec 4, 2020 • edited Loading

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

minwoox Dec 7, 2020

Choose a reason for hiding this comment

ikhoon Dec 7, 2020

Choose a reason for hiding this comment

minwoox Dec 7, 2020

Choose a reason for hiding this comment

minwoox Dec 7, 2020

Choose a reason for hiding this comment

ikhoon commented Dec 7, 2020 • edited Loading

minwoox commented Dec 7, 2020

ikhoon commented Dec 7, 2020

minwoox left a comment • edited Loading

Choose a reason for hiding this comment

minwoox Dec 8, 2020

Choose a reason for hiding this comment

ikhoon Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

ikhoon commented Dec 8, 2020

ikhoon commented Dec 8, 2020

minwoox commented Dec 8, 2020

ikhoon commented Dec 9, 2020

minwoox commented Dec 10, 2020

ikhoon commented Dec 10, 2020

ikhoon commented Nov 20, 2020 •

edited

Loading

codecov bot commented Dec 4, 2020 •

edited

Loading

ikhoon commented Dec 7, 2020 •

edited

Loading

minwoox left a comment •

edited

Loading

ikhoon Dec 8, 2020 •

edited

Loading