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.

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

Introduce the resource control client #648

Merged
merged 13 commits into from Jan 20, 2023
Merged

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Dec 26, 2022

Introduce the resource control client and its interceptor

Should merge after tikv/pd#5811.

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

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

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.

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
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
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested a review from nolouch January 18, 2023 12:17
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
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
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
tikv/kv.go Outdated Show resolved Hide resolved
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
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@BornChanger
Copy link
Contributor

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

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato marked this pull request as ready for review January 19, 2023 12:10
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@BornChanger
Copy link
Contributor

Ref pingcap/tidb#38825

Signed-off-by: JmPotato <ghzpotato@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants