Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

client: Count resource penalty for resource control #6336

Merged
merged 18 commits into from
Apr 19, 2023
Merged

client: Count resource penalty for resource control #6336

merged 18 commits into from
Apr 19, 2023

Conversation

Connor1996
Copy link
Member

What problem does this PR solve?

Issue Number: Close #6335

What is changed and how does it work?

Count the delta resource consumption of the resource group that have completed at all stores between the previous request to this store and current request.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Increased code complexity

Release note

Implement dmclock algorithm for resource control

@Connor1996
record delta
106b5cd 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fix build
8396651 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fix panic
59be299 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
add lock
5ca0ce9 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fix update
17de08f 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fine grained
c530690 
Signed-off-by: Connor1996 <[email protected]>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 18, 2023 •
edited
Loading

[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 ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 18, 2023
@Connor1996 Connor1996 requested a review from nolouch April 18, 2023 09:21
@ti-chi-bot ti-chi-bot requested a review from rleungx April 18, 2023 09:21
@Connor1996
Copy link
Member Author

PTAL @glorv

@Connor1996
address comment
1858403 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fix and add test
ee162b6 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
make format
9ff587b 
Signed-off-by: Connor1996 <[email protected]>
glorv
glorv reviewed Apr 19, 2023
View reviewed changes
Copy link
Contributor

@glorv glorv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Connor1996
fix style
dfcac87 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
refactor
3c74095 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
Copy link
Member Author

PTAL again @glorv @nolouch

@Connor1996 Connor1996 changed the title client: Count resource delta for resource control client: Count resource penalty for resource control Apr 19, 2023
@nolouch nolouch requested a review from CabinfeverB April 19, 2023 07:01
@Connor1996
address comment
d36e37c 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
clean
33961e5 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
make format
7e7b15f 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
fix test build
4ee343e 
Signed-off-by: Connor1996 <[email protected]>
@Connor1996
tear down test
69e76bd 
Signed-off-by: Connor1996 <[email protected]>
CabinfeverB
CabinfeverB reviewed Apr 19, 2023
View reviewed changes
client/resource_group/controller/controller.go
@@ -465,7 +465,9 @@ type groupCostController struct {

mu struct {
sync.Mutex
consumption *rmpb.Consumption
consumption *rmpb.Consumption
storeCounter map[uint64]*rmpb.Consumption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How add adding a lock? The logic for storeCounter and globalCounter is independent of consumption

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but they almost use the lock at the same time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a hot path. I guess two locks will help get better performance. But maybe it's not the bottleneck currently

@Connor1996
fix delta
161cc6f 
Signed-off-by: Connor1996 <[email protected]>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 19, 2023
@codecov
Copy link

codecov bot commented Apr 19, 2023 •
edited
Loading

Codecov Report

Patch coverage: 62.50% and project coverage change: +0.12 🎉

Comparison is base (1c360b6) 75.02% compared to head (161cc6f) 75.15%.

❗ Current head 161cc6f differs from pull request most recent head cf7c977. Consider uploading reports for the commit cf7c977 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6336      +/-   ##
==========================================
+ Coverage   75.02%   75.15%   +0.12%     
==========================================
  Files         408      408              
  Lines       40363    40381      +18     
==========================================
+ Hits        30284    30347      +63     
+ Misses       7429     7393      -36     
+ Partials     2650     2641       -9
Flag Coverage Δ
unittests 75.15% <62.50%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
client/resource_group/controller/model.go 73.68% <ø> (+6.57%) ⬆️
client/resource_group/controller/controller.go 73.65% <57.14%> (+0.77%) ⬆️
client/resource_group/controller/testutil.go 100.00% <100.00%> (ø)

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

@ti-chi-bot 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 Apr 19, 2023
@nolouch
Copy link
Contributor

nolouch commented Apr 19, 2023

/merge

@ti-chi-bot
Copy link
Member

@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
Copy link
Member

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

Commit hash: 161cc6f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 19, 2023
@Connor1996
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

@Connor1996: 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.

@CabinfeverB
Copy link
Member

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit 9e7b8e8 into tikv:master Apr 19, 2023
@Connor1996 Connor1996 mentioned this pull request Apr 19, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glorv glorv glorv left review comments

@CabinfeverB CabinfeverB CabinfeverB approved these changes

@nolouch nolouch nolouch approved these changes

@rleungx rleungx Awaiting requested review from rleungx

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resource control is not ideal when the request load from different resource group is unbalanced
5 participants
@Connor1996 @ti-chi-bot @nolouch @CabinfeverB @glorv