mcs/resourcemanager: delete expire tokenSlot #7344

guo-shaoge · 2023-11-09T12:17:33Z

What problem does this PR solve?

If tidb/tiflash crash unexpectly or killed by signal which cannot be handled, there will be a useless slot, which will cause fillrate/burstlimit become less than expected for other alive ones.
So add a expire timeout(10min).

Issue Number: Close #7346

What is changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

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

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: guo-shaoge <[email protected]>

ti-chi-bot · 2023-11-09T12:17:36Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
HuSharp

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.

Signed-off-by: guo-shaoge <[email protected]>

…o del_expire_slot

codecov · 2023-11-09T13:39:14Z

Codecov Report

Merging #7344 (473b423) into master (0c35227) will increase coverage by 0.07%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master    #7344      +/-   ##
==========================================
+ Coverage   74.36%   74.44%   +0.07%     
==========================================
  Files         446      446              
  Lines       48441    48451      +10     
==========================================
+ Hits        36025    36071      +46     
+ Misses       9234     9190      -44     
- Partials     3182     3190       +8

Flag	Coverage Δ
unittests	`74.44% <63.63%> (+0.07%)`	⬆️

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

Signed-off-by: guo-shaoge <[email protected]>

CabinfeverB · 2023-11-10T02:22:16Z

pkg/mcs/resourcemanager/server/token_buckets.go

@@ -139,6 +147,13 @@ func (gts *GroupTokenBucketState) balanceSlotTokens(
 		}
 	}

+	for clientUniqueID, slot := range gts.tokenSlots {


Do we need check this every handling request?

should refine so we can check every 10mins

Add gts.lastCheckExpireSlot as a guard

Signed-off-by: guo-shaoge <[email protected]>

HuSharp · 2023-11-10T03:32:15Z

pkg/mcs/resourcemanager/server/token_buckets.go

+	now := time.Now()
+	gts.lastCheckExpireSlot = now


Do we need to check expiration here first?

deleted, no need to update here

Signed-off-by: guo-shaoge <[email protected]>

pkg/mcs/resourcemanager/server/token_buckets.go

HuSharp

rest LGTM!

HuSharp · 2023-11-10T05:48:50Z

seems like DCO is wrong with sign-off is missing :) @guo-shaoge

Commit sha: [6122ee9](https://github.com/tikv/pd/pull/7344/commits/6122ee9c413627d4027da7dc0e80b1e6a9c3dd70), Author: guo-shaoge, Committer: GitHub; The sign-off is missing.
Commit sha: [7201cce](https://github.com/tikv/pd/pull/7344/commits/7201cce1cfabad5d42c2f87efac6510fe2453356), Author: guo-shaoge, Committer: GitHub; The sign-off is missing.

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge · 2023-11-10T05:50:27Z

seems like DCO is wrong with sign-off is missing :) @guo-shaoge

Commit sha: [6122ee9](https://github.com/tikv/pd/pull/7344/commits/6122ee9c413627d4027da7dc0e80b1e6a9c3dd70), Author: guo-shaoge, Committer: GitHub; The sign-off is missing.
Commit sha: [7201cce](https://github.com/tikv/pd/pull/7344/commits/7201cce1cfabad5d42c2f87efac6510fe2453356), Author: guo-shaoge, Committer: GitHub; The sign-off is missing.

fixed

CabinfeverB · 2023-11-10T06:00:07Z

No test in this PR, can u do a manual test? @guo-shaoge
I also agree to merge first and make up later

guo-shaoge · 2023-11-10T07:46:16Z

No test in this PR, can u do a manual test? @guo-shaoge I also agree to merge first and make up later

tested last night.

setup cluster
kill -9 tiflash
wait 10mins and check pd log

CabinfeverB · 2023-11-10T07:47:15Z

/merge

ti-chi-bot · 2023-11-10T07:47:18Z

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

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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-11-10T07:47:19Z

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

Commit hash: 473b423

ti-chi-bot · 2023-11-10T07:49:20Z

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

ti-chi-bot · 2023-11-10T07:49:57Z

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

close #7346 Signed-off-by: guo-shaoge <[email protected]> Co-authored-by: guo-shaoge <[email protected]>

close #7346 Signed-off-by: guo-shaoge <[email protected]> Co-authored-by: guo-shaoge <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

mcs/resourcemanager: delete expire tokenSlot

427e76c

Signed-off-by: guo-shaoge <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Nov 9, 2023

ti-chi-bot bot requested review from disksing and JmPotato November 9, 2023 12:17

ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 9, 2023

guo-shaoge added 3 commits November 9, 2023 20:56

Merge branch 'master' into del_expire_slot

bec4ce5

log

42ff1a7

Signed-off-by: guo-shaoge <[email protected]>

Mergh origin branch 'del_expire_slot' of github.com:guo-shaoge/pd int…

30577d8

…o del_expire_slot

guo-shaoge added 3 commits November 9, 2023 22:45

log

28bec19

Signed-off-by: guo-shaoge <[email protected]>

refine

229bcf0

Signed-off-by: guo-shaoge <[email protected]>

refine

647e878

Signed-off-by: guo-shaoge <[email protected]>

HuSharp requested review from CabinfeverB and HuSharp and removed request for JmPotato and disksing November 10, 2023 02:10

CabinfeverB reviewed Nov 10, 2023

View reviewed changes

guo-shaoge added 2 commits November 10, 2023 10:38

only check each 10 mins

5227b33

Signed-off-by: guo-shaoge <[email protected]>

refine

86e202e

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge requested a review from CabinfeverB November 10, 2023 02:42

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Nov 10, 2023

HuSharp reviewed Nov 10, 2023

View reviewed changes

update resetLoan()

dfdf2f7

Signed-off-by: guo-shaoge <[email protected]>

ti-chi-bot bot added needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. and removed do-not-merge/needs-triage-completed labels Nov 10, 2023

CabinfeverB approved these changes Nov 10, 2023

View reviewed changes

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

guo-shaoge requested a review from HuSharp November 10, 2023 05:22

HuSharp reviewed Nov 10, 2023

View reviewed changes

pkg/mcs/resourcemanager/server/token_buckets.go Outdated Show resolved Hide resolved

HuSharp reviewed Nov 10, 2023

View reviewed changes

pkg/mcs/resourcemanager/server/token_buckets.go Outdated Show resolved Hide resolved

HuSharp approved these changes Nov 10, 2023

View reviewed changes

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

update by comments

c179d78

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge force-pushed the del_expire_slot branch from 7201cce to c179d78 Compare November 10, 2023 05:49

Merge branch 'master' into del_expire_slot

473b423

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

ti-chi-bot bot merged commit f1cee6c into tikv:master Nov 10, 2023
24 of 26 checks passed

This was referenced Nov 10, 2023

mcs/resourcemanager: delete expire tokenSlot (#7344) #7349

Merged

mcs/resourcemanager: delete expire tokenSlot (#7344) #7350

Merged

CabinfeverB mentioned this pull request Nov 10, 2023

remove legacy tokenSlot of resource group bucket #7346

Open

ti-chi-bot bot pushed a commit that referenced this pull request Nov 10, 2023

mcs/resourcemanager: delete expire tokenSlot (#7344) (#7350)

d09a4f5

close #7346 Signed-off-by: guo-shaoge <[email protected]> Co-authored-by: guo-shaoge <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs/resourcemanager: delete expire tokenSlot #7344

mcs/resourcemanager: delete expire tokenSlot #7344

guo-shaoge commented Nov 9, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

CabinfeverB Nov 10, 2023

guo-shaoge Nov 10, 2023

guo-shaoge Nov 10, 2023

HuSharp Nov 10, 2023

guo-shaoge Nov 10, 2023

HuSharp left a comment

HuSharp commented Nov 10, 2023 •

edited

Loading

guo-shaoge commented Nov 10, 2023

CabinfeverB commented Nov 10, 2023 •

edited

Loading

guo-shaoge commented Nov 10, 2023

CabinfeverB commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

mcs/resourcemanager: delete expire tokenSlot #7344

mcs/resourcemanager: delete expire tokenSlot #7344

Conversation

guo-shaoge commented Nov 9, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 9, 2023 • edited Loading

codecov bot commented Nov 9, 2023 • edited Loading

Codecov Report

CabinfeverB Nov 10, 2023

Choose a reason for hiding this comment

guo-shaoge Nov 10, 2023

Choose a reason for hiding this comment

guo-shaoge Nov 10, 2023

Choose a reason for hiding this comment

HuSharp Nov 10, 2023

Choose a reason for hiding this comment

guo-shaoge Nov 10, 2023

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

HuSharp commented Nov 10, 2023 • edited Loading

guo-shaoge commented Nov 10, 2023

CabinfeverB commented Nov 10, 2023 • edited Loading

guo-shaoge commented Nov 10, 2023

CabinfeverB commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

guo-shaoge commented Nov 9, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

HuSharp commented Nov 10, 2023 •

edited

Loading

CabinfeverB commented Nov 10, 2023 •

edited

Loading