resource_manager/client: Don't set token as `-1` in burstable mode #6216

CabinfeverB · 2023-03-23T08:20:22Z

What problem does this PR solve?

Issue Number: Close #6209

What is changed and how does it work?

Before this PR, the token of limiter is set to -1 in burst mode. But ref #6209, after stopping workload, the avg will be zore. So the required token will be zero. If we disable burstable, the token and fillratre are both zero, so workload can not run.
This PR block token refill when in burstable mode and keep the token as FillRate.

Check List

Tests

Integration test
Manual test (add detailed scripts or steps below)

mysql> alter resource group rg1 RU_PER_SEC=10000 BURSTABLE;
run sysbench and kill it.
wait 1 minute and alter resource group rg1 RU_PER_SEC=1000;
run sysbench again.

Release note

None.

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot · 2023-03-23T08:20:25Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
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.

codecov · 2023-03-23T08:34:07Z

Codecov Report

Patch coverage: 73.68% and project coverage change: +0.06 🎉

Comparison is base (19f7dd9) 74.71% compared to head (279124d) 74.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6216      +/-   ##
==========================================
+ Coverage   74.71%   74.77%   +0.06%     
==========================================
  Files         395      395              
  Lines       38826    38846      +20     
==========================================
+ Hits        29008    29048      +40     
+ Misses       7274     7261      -13     
+ Partials     2544     2537       -7

Flag	Coverage Δ
unittests	`74.77% <73.68%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
client/resource_group/controller/controller.go	`73.59% <72.72%> (+0.85%)`	⬆️
client/resource_group/controller/limiter.go	`72.35% <75.00%> (-0.04%)`	⬇️

... and 28 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.

HuSharp · 2023-03-23T09:06:15Z

client/resource_group/controller/controller.go

-		fillRate := getRUTokenBucketSetting(gc.ResourceGroup, typ)
-		cfg.NewBurst = int64(fillRate.Settings.FillRate)
-		cfg.NewRate = float64(fillRate.Settings.FillRate)
+		tb := counter.getTokenBucketFunc()


maybe can directly get fillrate?

HuSharp · 2023-03-23T09:06:41Z

client/resource_group/controller/controller.go

-		fillRate := getRawResourceTokenBucketSetting(gc.ResourceGroup, typ)
-		cfg.NewBurst = int64(fillRate.Settings.FillRate)
-		cfg.NewRate = float64(fillRate.Settings.FillRate)
+		tb := counter.getTokenBucketFunc()


nolouch · 2023-03-23T09:55:18Z

client/resource_group/controller/limiter.go

 	lim.limit = Limit(args.NewRate)
 	lim.burst = args.NewBurst
+	_, _, lim.tokens = lim.advance(now)


why not advance before lim.last = now?

I was trying to avoid accumulating too many tokens when burst is -1. Because the first advance was before the burst changes, we want to advance after the burst changes.
But I feel this worry is unnecessary, I will delete this line

Signed-off-by: Cabinfever_B <[email protected]>

client/resource_group/controller/controller.go

nolouch

lgtm

HuSharp · 2023-03-24T01:52:21Z

tests/mcs/resource_manager/resource_manager_test.go

@@ -334,6 +334,114 @@ func (suite *resourceManagerClientTestSuite) TestResourceGroupController() {
 	controller.Stop()
 }

+// TestSwitchBurst is used to test https://github.com/tikv/pd/issue/6209


#6209
issues miss a s :)

Signed-off-by: Cabinfever_B <[email protected]>

HuSharp

REST LGTM!

HuSharp · 2023-03-24T02:21:20Z

client/resource_group/controller/controller.go

-		// In the non-trickle case, clients can be allowed to accumulate more tokens.
-		if cfg.NewBurst >= 0 {
-			cfg.NewBurst = 0
+		cfg.NotifyThreshold = math.Min((granted+counter.limiter.AvailableTokens(gc.run.now)), counter.avgRUPerSec*defaultTargetPeriod.Seconds()) * notifyFraction


redundant bracket

HuSharp · 2023-03-24T02:45:54Z

client/resource_group/controller/limiter.go

-	lim.tokens = tokens + args.NewTokens
+	log.Debug("[resource group controller] before reconfigure", zap.Float64("NewTokens", lim.tokens), zap.Float64("NewRate", float64(lim.limit)), zap.Float64("NotifyThreshold", args.NotifyThreshold), zap.Int64("burst", lim.burst))
+	if args.NewBurst < 0 {
+		lim.tokens = args.NewTokens


The annotation for the limiter field last is last is the last time the token field of the limiter was updated
So for this change do we need to update annotation or will this cause some unexpect behavior?

I changed it and will update last here as well

ti-chi-bot · 2023-03-24T02:46:16Z

@HuSharp: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

REST LGTM!

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.

Signed-off-by: Cabinfever_B <[email protected]>

JmPotato · 2023-03-24T03:33:01Z

/merge

ti-chi-bot · 2023-03-24T03:33:03Z

@JmPotato: 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-24T03:33:04Z

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

Commit hash: 279124d

ti-chi-bot · 2023-03-24T03:35:21Z

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

…ikv#6216) close tikv#6209, ref tikv#6209 Signed-off-by: Cabinfever_B <[email protected]>

…6216) (#6220) close #6209, ref #6209, ref #6216 Signed-off-by: Cabinfever_B <[email protected]> Co-authored-by: Yongbo Jiang <[email protected]>

CabinfeverB added 4 commits March 23, 2023 10:50

fix burstable remove token

f498bd5

Signed-off-by: Cabinfever_B <[email protected]>

fix burstable remove token

bdc3e4b

Signed-off-by: Cabinfever_B <[email protected]>

fix burstable remove token

e6f5a9c

Signed-off-by: Cabinfever_B <[email protected]>

fix burstable remove token

bcf7aa2

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-7.0 labels Mar 23, 2023

ti-chi-bot requested review from JmPotato and nolouch March 23, 2023 08:20

Merge branch 'master' into resource-manager/fix_burst

97dbe29

HuSharp reviewed Mar 23, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2023

nolouch reviewed Mar 23, 2023

View reviewed changes

CabinfeverB added 2 commits March 23, 2023 19:41

add test case

eedd4c6

Signed-off-by: Cabinfever_B <[email protected]>

merge master

582473e

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2023

nolouch reviewed Mar 23, 2023

View reviewed changes

client/resource_group/controller/controller.go Show resolved Hide resolved

nolouch approved these changes Mar 23, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 23, 2023

HuSharp reviewed Mar 24, 2023

View reviewed changes

address comment

fd2641e

Signed-off-by: Cabinfever_B <[email protected]>

HuSharp approved these changes Mar 24, 2023

View reviewed changes

address comment

279124d

Signed-off-by: Cabinfever_B <[email protected]>

JmPotato approved these changes Mar 24, 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 24, 2023

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

ti-chi-bot merged commit 79ec29c into tikv:master Mar 24, 2023

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

resource_manager/client: Don't set token as -1 in burstable mode (#6216) #6220

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Mar 24, 2023

resource_manager/client: Don't set token as -1 in burstable mode (t…

c098739

…ikv#6216) close tikv#6209, ref tikv#6209 Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB added a commit to ti-chi-bot/pd that referenced this pull request Mar 24, 2023

resource_manager/client: Don't set token as -1 in burstable mode (t…

7436df3

…ikv#6216) close tikv#6209, ref tikv#6209 Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB mentioned this pull request Mar 24, 2023

mod: update pd client pingcap/tidb#42544

Merged

12 tasks

JmPotato deleted the resource-manager/fix_burst branch March 24, 2023 05:18

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

mod: update pd client (#42544) pingcap/tidb#42556

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager/client: Don't set token as `-1` in burstable mode #6216

resource_manager/client: Don't set token as `-1` in burstable mode #6216

CabinfeverB commented Mar 23, 2023 •

edited

Loading

ti-chi-bot commented Mar 23, 2023 •

edited

Loading

codecov bot commented Mar 23, 2023 •

edited

Loading

HuSharp Mar 23, 2023

CabinfeverB Mar 23, 2023

HuSharp Mar 23, 2023

nolouch Mar 23, 2023

CabinfeverB Mar 23, 2023

nolouch left a comment

HuSharp Mar 24, 2023

HuSharp left a comment

HuSharp Mar 24, 2023 •

edited

Loading

HuSharp Mar 24, 2023 •

edited

Loading

CabinfeverB Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

JmPotato commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

resource_manager/client: Don't set token as -1 in burstable mode #6216

resource_manager/client: Don't set token as -1 in burstable mode #6216

Conversation

CabinfeverB commented Mar 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 Mar 23, 2023 • edited Loading

codecov bot commented Mar 23, 2023 • edited Loading

Codecov Report

HuSharp Mar 23, 2023

Choose a reason for hiding this comment

CabinfeverB Mar 23, 2023

Choose a reason for hiding this comment

HuSharp Mar 23, 2023

Choose a reason for hiding this comment

nolouch Mar 23, 2023

Choose a reason for hiding this comment

CabinfeverB Mar 23, 2023

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

HuSharp Mar 24, 2023

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

HuSharp Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

HuSharp Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

CabinfeverB Mar 24, 2023

Choose a reason for hiding this comment

ti-chi-bot commented Mar 24, 2023

JmPotato commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

resource_manager/client: Don't set token as `-1` in burstable mode #6216

resource_manager/client: Don't set token as `-1` in burstable mode #6216

CabinfeverB commented Mar 23, 2023 •

edited

Loading

ti-chi-bot commented Mar 23, 2023 •

edited

Loading

codecov bot commented Mar 23, 2023 •

edited

Loading

HuSharp Mar 24, 2023 •

edited

Loading

HuSharp Mar 24, 2023 •

edited

Loading