resourcecontrol: settle batched cop tasks' execution details#2032
resourcecontrol: settle batched cop tasks' execution details#20320xTars wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @0xTars. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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 kubernetes-sigs/prow repository. |
|
Welcome @0xTars! |
Pin the existing MakeResponseInfo behavior through its public seam before changing resource accounting. The fixture records that only top-level scan bytes and KV CPU are currently settled in both legacy and NextGen builds. Signed-off-by: 0xTars <1196089730@qq.com>
Move the existing legacy and NextGen scan-byte rules into one helper without changing behavior. The helper lets top-level and nested responses share the same compatibility policy in the subsequent accounting change. Signed-off-by: 0xTars <1196089730@qq.com>
Include every nested coprocessor response when settling read bytes and KV CPU. Use scan-detail bytes when present and response data size otherwise, preserving the existing legacy and NextGen policies for both top-level and nested work. Signed-off-by: 0xTars <1196089730@qq.com>
2ddeed3 to
2c6b86f
Compare
Problem
MakeResponseInforeads execution details only from the top-level coprocessor response. AStoreBatchTasksresponse represents several logical tasks, and each attached batch response carries its own scan detail and process time. Ignoring those children undercounts read bytes and KV CPU during resource-group settlement and RU-based runaway detection.This replaces #2029 because its source branch cannot be updated.
What changed
Sum every attached batch response's execution details into the response information used for resource-control settlement. When a child has no scan detail, use its data length as the same fallback already used for the top-level response.
The replacement branch is based on TiDB's existing pinned client-go revision, so updating that consumer adds only this patch rather than unrelated newer client changes. Production behavior is identical to #2029; the tests and comments were revised to make the ordinary and NextGen accounting rules explicit.
Compatibility
There is no public API or wire-format change. Non-batched responses retain their existing accounting. Resource usage for batched coprocessor responses can increase to include work that was previously omitted; this is the intended compatibility impact for TiDB resource control. TiCDC, BR, CSE, and other consumers are unaffected unless they use this internal batched-response settlement path.
Tests
go test -mod=readonly ./internal/client ./internal/resourcecontrol -count=1go test -mod=readonly -tags nextgen ./internal/client ./internal/resourcecontrol -count=1MakeResponseInfotests with and without thenextgenbuild taggo generate ./...(clean diff)git diff --checkgofmton the changed filesRelated changes