resource_manager: impl limiter for token controlloer #5868

CabinfeverB · 2023-01-15T18:26:47Z

What problem does this PR solve?

Issue Number: ref #5851

What is changed and how does it work?

Based on the rate limiter implemented by Go(ref to https://cs.opensource.google/go/x/time/+/refs/tags/v0.2.0:rate/rate.go), some modifications have been made.
The first is the introduction of token notification, which is sent to the channel when the threshold is below.
Second, only the Reserve method is retained, and WaitReservations takes example by Wait method to handle multiple reservations. I also modified the reservation cancel function, once cancel, restore all the tokens.

impl limiter for token controlloer

Check List

Tests

Unit test

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2023-01-15T18:26:48Z

[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-01-15T18:32:28Z

Codecov Report

Base: 75.59% // Head: 75.41% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (d69d505) compared to base (296f062).
Patch coverage: 68.96% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5868      +/-   ##
==========================================
- Coverage   75.59%   75.41%   -0.19%     
==========================================
  Files         337      340       +3     
  Lines       34114    34290     +176     
==========================================
+ Hits        25790    25860      +70     
- Misses       6110     6200      +90     
- Partials     2214     2230      +16

Flag	Coverage Δ
unittests	`75.41% <68.96%> (-0.19%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/mcs/resource_manager/client/limiter.go	`68.96% <68.96%> (ø)`
pkg/dashboard/keyvisual/input/core.go	`0.00% <0.00%> (-33.34%)`	⬇️
server/tso/tso.go	`67.97% <0.00%> (-7.87%)`	⬇️
pkg/storage/kv/etcd_kv.go	`65.07% <0.00%> (-7.15%)`	⬇️
pkg/election/leadership.go	`75.25% <0.00%> (-5.16%)`	⬇️
pkg/member/member.go	`62.10% <0.00%> (-2.11%)`	⬇️
pkg/election/lease.go	`84.05% <0.00%> (-1.45%)`	⬇️
server/region_syncer/server.go	`81.86% <0.00%> (-1.10%)`	⬇️
server/schedule/operator_controller.go	`85.97% <0.00%> (-0.91%)`	⬇️
server/cluster/coordinator.go	`71.59% <0.00%> (-0.76%)`	⬇️
... and 14 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

nolouch

plz add the changes item in the pull request description.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch · 2023-01-17T04:11:43Z

pkg/mcs/resource_manager/client/limiter.go

+	// advance time to now
+	now, _, tokens := r.lim.advance(now)
+	// calculate new number of tokens
+	tokens += float64(r.tokens)


need address the lint check.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

rest LGTM

nolouch · 2023-01-17T08:22:49Z

pkg/mcs/resource_manager/client/limiter.go

+
+// reserveN is a helper method for Reserve.
+// maxFutureReserve specifies the maximum reservation wait duration allowed.
+// reserveN returns Reservation, not *Reservation, to avoid allocation in AllowN and WaitN.


Suggested change

// reserveN returns Reservation, not *Reservation, to avoid allocation in AllowN and WaitN.

// reserveN returns Reservation, not *Reservation.

nolouch · 2023-01-17T09:02:50Z

pkg/mcs/resource_manager/client/limiter.go

+		return Reservation{
+			ok:        false,
+			lim:       lim,
+			tokens:    lim.tokens,


ok is false, is the tokens field useless?

nolouch · 2023-01-17T09:06:55Z

pkg/mcs/resource_manager/client/limiter.go

+			tokens:    n,
+			timeToAct: now,
+		}
+	} else if n > maxRequestTokens {


I think maxRequestTokens should be configuable , or no need maxRequestTokens in client side.

JmPotato · 2023-01-17T09:35:21Z

pkg/mcs/resource_manager/client/limiter.go

+}
+
+// SetupNotificationAt enables the notification at the given threshold.
+func (lim *Limiter) SetupNotificationAt(now time.Time, threshold float64) {


Using the word At is a little bit confusing for the caller. They may be confused about which parameter this At refers to.

Suggested change

func (lim *Limiter) SetupNotificationAt(now time.Time, threshold float64) {

func (lim *Limiter) SetupNotificationThreshold(now time.Time, threshold float64) {

JmPotato · 2023-01-17T09:37:58Z

pkg/mcs/resource_manager/client/limiter.go

+	last                time.Time
+	notifyThreshold     float64
+	lowTokensNotifyChan chan struct{}
+	isLowProcess        bool


What does this field mean? Adding a comment will be helpful.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

JmPotato · 2023-01-17T12:35:13Z

/merge

ti-chi-bot · 2023-01-17T12:35:14Z

@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-01-17T12:35:16Z

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

Commit hash: 122c52b

CabinfeverB added 2 commits January 16, 2023 02:24

add limtier

f1bc87b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

d74e600

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 15, 2023

ti-chi-bot requested review from HunDunDM and lhy1024 January 15, 2023 18:26

CabinfeverB requested review from JmPotato and nolouch and removed request for lhy1024 and HunDunDM January 15, 2023 18:27

nolouch reviewed Jan 16, 2023

View reviewed changes

remove useless code

d68fd7f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Jan 17, 2023

View reviewed changes

nolouch mentioned this pull request Jan 17, 2023

Global Resource Quota Control pingcap/tidb#38825

Closed

41 tasks

address comment

ac83f2c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Jan 17, 2023

View reviewed changes

JmPotato reviewed Jan 17, 2023

View reviewed changes

CabinfeverB added 3 commits January 17, 2023 18:20

address comment and add test

57d526f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment and add test

0c46eaf

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix test

122c52b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Jan 17, 2023

View reviewed changes

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

JmPotato approved these changes Jan 17, 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 Jan 17, 2023

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

Merge branch 'master' into resource-manager/limiter

d69d505

ti-chi-bot merged commit b2a0532 into tikv:master Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager: impl limiter for token controlloer #5868

resource_manager: impl limiter for token controlloer #5868

CabinfeverB commented Jan 15, 2023 •

edited

Loading

ti-chi-bot commented Jan 15, 2023 •

edited

Loading

codecov bot commented Jan 15, 2023 •

edited

Loading

nolouch left a comment

nolouch Jan 17, 2023

nolouch left a comment

nolouch Jan 17, 2023

nolouch Jan 17, 2023

nolouch Jan 17, 2023

JmPotato Jan 17, 2023

JmPotato Jan 17, 2023

nolouch left a comment

JmPotato commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

	// reserveN returns Reservation, not *Reservation, to avoid allocation in AllowN and WaitN.
	// reserveN returns Reservation, not *Reservation.

	func (lim *Limiter) SetupNotificationAt(now time.Time, threshold float64) {
	func (lim *Limiter) SetupNotificationThreshold(now time.Time, threshold float64) {

resource_manager: impl limiter for token controlloer #5868

resource_manager: impl limiter for token controlloer #5868

Conversation

CabinfeverB commented Jan 15, 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 Jan 15, 2023 • edited Loading

codecov bot commented Jan 15, 2023 • edited Loading

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

nolouch Jan 17, 2023

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch Jan 17, 2023

Choose a reason for hiding this comment

nolouch Jan 17, 2023

Choose a reason for hiding this comment

nolouch Jan 17, 2023

Choose a reason for hiding this comment

JmPotato Jan 17, 2023

Choose a reason for hiding this comment

JmPotato Jan 17, 2023

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

JmPotato commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

CabinfeverB commented Jan 15, 2023 •

edited

Loading

ti-chi-bot commented Jan 15, 2023 •

edited

Loading

codecov bot commented Jan 15, 2023 •

edited

Loading