resource_manager: implement the token-consuming by client #6039

HuSharp · 2023-02-23T08:17:01Z

What problem does this PR solve?

Issue Number: close #5947

What is changed and how does it work?

- Introduce the token slot to balance the QPS within the same resource group.

Check List

Tests

Unit test
Integration test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Release note

None.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot · 2023-02-23T08:17:03Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-02-23T08:17:10Z

Hi @HuSharp. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lhy1024 · 2023-02-23T08:49:14Z

/ok-to-test

Signed-off-by: husharp <jinhao.hu@pingcap.com>

codecov · 2023-02-23T10:51:25Z

Codecov Report

Patch coverage: 72.43% and project coverage change: -0.26 ⚠️

Comparison is base (c67e90a) 74.94% compared to head (5d73b28) 74.69%.

❗ Current head 5d73b28 differs from pull request most recent head ceec274. Consider uploading reports for the commit ceec274 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6039      +/-   ##
==========================================
- Coverage   74.94%   74.69%   -0.26%     
==========================================
  Files         395      395              
  Lines       38770    38760      -10     
==========================================
- Hits        29058    28953     -105     
- Misses       7192     7259      +67     
- Partials     2520     2548      +28

Flag	Coverage Δ
unittests	`74.69% <72.43%> (-0.26%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
client/resource_group/controller/limiter.go	`72.39% <ø> (ø)`
pkg/mcs/resource_manager/server/manager.go	`80.23% <50.00%> (-4.68%)`	⬇️
pkg/mcs/resource_manager/server/resource_group.go	`70.32% <62.50%> (-0.05%)`	⬇️
pkg/mcs/resource_manager/server/token_bukets.go	`77.15% <73.71%> (-14.85%)`	⬇️
client/resource_group/controller/controller.go	`71.97% <100.00%> (-0.77%)`	⬇️
pkg/mcs/resource_manager/server/apis/v1/api.go	`77.58% <100.00%> (-8.86%)`	⬇️
pkg/mcs/resource_manager/server/grpc_service.go	`65.55% <100.00%> (-4.45%)`	⬇️

... and 36 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

CabinfeverB · 2023-03-22T04:34:15Z

pkg/mcs/resource_manager/server/token_bukets.go

@@ -30,6 +30,7 @@ const (
 const (
 	defaultReserveRatio    = 0.05


Now that we've implemented slot, can we make defaultReserveRatio bigger?

modify to 0.5

Signed-off-by: husharp <jinhao.hu@pingcap.com>

pkg/mcs/resource_manager/server/token_bukets.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch · 2023-03-22T03:16:49Z

pkg/mcs/resource_manager/server/resource_group.go

-		if settings := group.GetRUSettings(); settings != nil {
-			rg.RUSettings = NewRequestUnitSettings(settings.GetRU())
+		if group.GetRUSettings() == nil {
+			rg.RUSettings = NewRequestUnitSettings(nil)


Will this cause some unexpect behavior?

I found CheckAndInit has redundant code and function with FromProtoResourceGroup,
So i replace CheckAndInit with FromProtoResourceGroup.

for CheckAndInit can keep original logic

for other way which call FromProtoResourceGroup, if set nil, just keep it.
And RequestRU will check it.

So i think it will be ok.

nolouch

lgtm

nolouch · 2023-03-23T08:30:45Z

/merge

ti-chi-bot · 2023-03-23T08:30:46Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-23T08:30:48Z

This pull request has been accepted and is ready to merge.

Commit hash: 5d73b28

Signed-off-by: husharp <jinhao.hu@pingcap.com>

CabinfeverB · 2023-03-23T09:14:11Z

/merge

ti-chi-bot · 2023-03-23T09:14:13Z

@CabinfeverB: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-23T09:14:15Z

This pull request has been accepted and is ready to merge.

Commit hash: ceec274

ti-chi-bot · 2023-03-23T09:15:21Z

In response to a cherrypick label: new pull request created to branch release-7.0: #6218.

close tikv#5947 - Introduce the token slot to balance the QPS within the same resource group. Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

close tikv#5947 - Introduce the token slot to balance the QPS within the same resource group. Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: husharp <jinhao.hu@pingcap.com>

close tikv#5947 - Introduce the token slot to balance the QPS within the same resource group. Signed-off-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

) close #5947, ref #6039 - Introduce the token slot to balance the QPS within the same resource group. Signed-off-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

roughly impl

2c3ed91

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot requested review from JmPotato and lhy1024 February 23, 2023 08:17

ti-chi-bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Feb 23, 2023

JmPotato requested review from CabinfeverB and nolouch and removed request for lhy1024 February 23, 2023 09:02

HuSharp added 2 commits February 23, 2023 17:42

make check happy

b73f7db

Signed-off-by: husharp <jinhao.hu@pingcap.com>

make check happy

0c6ceb2

Signed-off-by: husharp <jinhao.hu@pingcap.com>

move sync

0b56890

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the token_client_limit branch 4 times, most recently from 883415a to 3ca42f5 Compare February 27, 2023 02:00

add client id

384406e

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the token_client_limit branch from 3ca42f5 to 384406e Compare February 27, 2023 08:03

Merge branch 'master' into token_client_limit

29fd6a6

HuSharp force-pushed the token_client_limit branch 4 times, most recently from 37b3a1e to ae36e6b Compare February 28, 2023 08:02

update config

3ec7cbf

Signed-off-by: husharp <jinhao.hu@pingcap.com>

CabinfeverB reviewed Mar 22, 2023

View reviewed changes

HuSharp added 2 commits March 22, 2023 13:50

update ratio to 0.5

86024e5

Signed-off-by: husharp <jinhao.hu@pingcap.com>

update test with ratio

e3c4045

Signed-off-by: husharp <jinhao.hu@pingcap.com>

CabinfeverB reviewed Mar 22, 2023

View reviewed changes

pkg/mcs/resource_manager/server/token_bukets.go Outdated Show resolved Hide resolved

update test with ratio

5d73b28

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch reviewed Mar 23, 2023

View reviewed changes

nolouch approved these changes Mar 23, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 23, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 23, 2023

ti-chi-bot and others added 2 commits March 23, 2023 16:30

Merge branch 'master' into token_client_limit

20d084c

make check happy

a30f6af

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 23, 2023

HuSharp added 2 commits March 23, 2023 16:54

remove error

b31fffc

Signed-off-by: husharp <jinhao.hu@pingcap.com>

remove error

ceec274

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 23, 2023

ti-chi-bot merged commit 19f7dd9 into tikv:master Mar 23, 2023

HuSharp deleted the token_client_limit branch March 23, 2023 09:15

ti-chi-bot mentioned this pull request Mar 23, 2023

resource_manager: implement the token-consuming by client (#6039) #6218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager: implement the token-consuming by client #6039

resource_manager: implement the token-consuming by client #6039

HuSharp commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023

lhy1024 commented Feb 23, 2023

codecov bot commented Feb 23, 2023 •

edited

Loading

CabinfeverB Mar 22, 2023

HuSharp Mar 22, 2023

nolouch Mar 22, 2023

HuSharp Mar 23, 2023 •

edited

Loading

nolouch left a comment

nolouch commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

CabinfeverB commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

resource_manager: implement the token-consuming by client #6039

resource_manager: implement the token-consuming by client #6039

Conversation

HuSharp commented Feb 23, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 23, 2023 • edited Loading

ti-chi-bot commented Feb 23, 2023

lhy1024 commented Feb 23, 2023

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

CabinfeverB Mar 22, 2023

Choose a reason for hiding this comment

HuSharp Mar 22, 2023

Choose a reason for hiding this comment

nolouch Mar 22, 2023

Choose a reason for hiding this comment

HuSharp Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

CabinfeverB commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

ti-chi-bot commented Mar 23, 2023

HuSharp commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

HuSharp Mar 23, 2023 •

edited

Loading