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

Introduce the resource control client #648

Merged
merged 13 commits into from
Jan 20, 2023
Merged

Introduce the resource control client #648

merged 13 commits into from
Jan 20, 2023

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Dec 26, 2022 •
edited
Loading

Introduce the resource control client and its interceptor

Should merge after tikv/pd#5811.

@nolouch nolouch mentioned this pull request Dec 27, 2022
Closed
41 tasks
glorv
glorv reviewed Jan 12, 2023
View reviewed changes
keyspace/resourcegroup/resourcemanager.go Outdated
return &ResponseInfo{readBytes: readBytes, kvCPUMs: kvCPUMs}
}

func getKVCPUMs(detailsV2 *kvrpcpb.ExecDetailsV2, details *kvrpcpb.ExecDetails) uint64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid wall time cpu is not so accurate here because it contains some wait time, so when the load is high, the same request will take longer to finish. @sticnarf Do you have any idea about this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Truely. If we want to get the CPU time used by a request, process_wall_time_ms is inaccurate. It is reasonable to me to add another field for the real CPU cost.

Copy link
Contributor

@BornChanger BornChanger Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is GetProcessWallTimeMs() - GetWaitWallTimeMs() good enough at present? @sticnarf

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. For example, coprocessor may suspend a long request and execute other queries. The suspending time is counted in PorcessWallTimeMs but not counted in WaitWallTimeMs.

Copy link
Contributor

@nolouch nolouch Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the CPU precision does not need to be very high, Let's provide a better method later.

@JmPotato
Introduce the resource control client
6ee294f 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato changed the title Implement RequestInfo and ResponseInfo for RU calculation Introduce the resource control client Jan 18, 2023
nolouch
nolouch reviewed Jan 18, 2023
View reviewed changes
internal/client/client_interceptor.go Outdated Show resolved Hide resolved
internal/resource_control/resource_control.go Outdated Show resolved Hide resolved
internal/client/client_interceptor.go Outdated Show resolved Hide resolved
@JmPotato
Address the comments
35b648c 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested a review from nolouch January 18, 2023 12:17
@JmPotato
Rename
494bebd 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Update the go.mod
d4ffd3d 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Merge the upstream
88f7301 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Introduce the switch
320af65 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested review from glorv and nolouch and removed request for glorv January 19, 2023 08:15
@JmPotato JmPotato force-pushed the ru-model branch 3 times, most recently from 955da78 to b570334 Compare January 19, 2023 08:31
@JmPotato
Fix the check
08694ef 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Fix the test
8553f26 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Export the switch
2a19c65 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
nolouch
nolouch reviewed Jan 19, 2023
View reviewed changes
tikv/kv.go Outdated Show resolved Hide resolved
nolouch
nolouch reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

rest lgtm

tikv/kv.go Outdated Show resolved Hide resolved
@JmPotato
Make the switch global
319f6b9 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@BornChanger
Copy link
Contributor

tikv/pd#5811 is ready. please continue the process

@JmPotato
Update the go.mod
ed6d507 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato marked this pull request as ready for review January 19, 2023 12:10
@JmPotato
Refine the code
f24b731 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@BornChanger
Copy link
Contributor

Ref pingcap/tidb#38825

@nolouch nolouch mentioned this pull request Jan 19, 2023
Merged
5 tasks
@JmPotato
Simplify the code
313152f 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@HuSharp HuSharp mentioned this pull request Jan 20, 2023
Merged
1 task
@zhangjinpeng87 zhangjinpeng87 merged commit f893837 into tikv:master Jan 20, 2023
@JmPotato JmPotato deleted the ru-model branch January 20, 2023 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sticnarf sticnarf sticnarf left review comments

@glorv glorv glorv left review comments

@BornChanger BornChanger BornChanger approved these changes

@CabinfeverB CabinfeverB CabinfeverB approved these changes

@nolouch nolouch nolouch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JmPotato @BornChanger @glorv @nolouch @sticnarf @CabinfeverB @zhangjinpeng87