ruv2: fill rpc counters in exec details#1933
Merged
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis PR separates TiKV read/write RPC counting (int64), updates RU v2 update API and its delta computation to use both counts and mutates ExecDetailsV2.RuV2, wires counting through client and streaming responses, and bumps several go.mod dependency versions. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Req as Request
participant TiKV as TiKV RPC
participant Resp as CopStreamResponse
participant Config as config.UpdateTiKVRUV2FromExecDetailsV2
Client->>Req: prepare request (sets CountRPC/Ctx for cop streams)
Client->>TiKV: send RPC
TiKV-->>Resp: stream response (Resp.CountRPC may be true)
Resp->>Resp: Recv() reads message
alt Resp.CountRPC == true
Resp->>Config: UpdateTiKVRUV2FromExecDetailsV2(Resp.Ctx, ExecDetailsV2, readRPCCount=1, writeRPCCount=0)
else
Resp->>Config: UpdateTiKVRUV2FromExecDetailsV2(Resp.Ctx, ExecDetailsV2, 0, 0)
end
Config->>Config: mutate ExecDetailsV2.RuV2, recompute delta, accumulate ruDetails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ti-chi-bot
Bot
added
the
size/L
disksing
force-pushed
the
disksing/rpc-interceptor
branch
2 times, most recently
from
b55278d to
8863bb9
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tikvrpc/tikvrpc.go (1)
1349-1359:⚠️ Potential issue | 🟠 MajorComplete the BatchCop stream accounting path.
internal/client/client.gonow setsCountRPConBatchCopStreamResponseand skips RUv2 updates insendRequest()because streams are expected to self-account inRecv(). This method only clears the flag, so batch-cop responses never propagate any RPC counters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tikvrpc/tikvrpc.go` around lines 1349 - 1359, BatchCopStreamResponse.Recv currently only clears the deadline and CountRPC flag but does not perform the RPC/accounting work streams are expected to do; copy the RPC accounting path from sendRequest() into Recv(): when ret != nil (i.e., a real BatchResponse is received) set resp.BatchResponse = ret, ensure resp.CountRPC is set so this response is charged, and invoke the same RPC counter/RUv2 update logic used in sendRequest() (the exact bookkeeping calls used there) so the stream propagates RPC counters and updates RU metrics for BatchCopStreamResponse.
🧹 Nitpick comments (1)
config/ruv2_test.go (1)
26-55: Add a nil-ExecDetailsV2fallback case.This only exercises the path where
ExecDetailsV2.RuV2already exists. A case withdetails == nilordetails.RuV2 == nilwould lock down the compatibility fallback for older responses now that raw RPC counts are being threaded throughExecDetailsV2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/ruv2_test.go` around lines 26 - 55, Add tests that exercise the nil-branch in UpdateTiKVRUV2FromExecDetailsV2 by calling it with details == nil and with &kvrpcpb.ExecDetailsV2{RuV2: nil} so the compatibility fallback path is covered; create a similar test setup as TestUpdateTiKVRUV2FromExecDetailsV2 (use DefaultConfig(), DefaultRUV2TiKVConfig(), StoreGlobalConfig, util.NewRUDetails() and ctx with util.RUDetailsCtxKey) and assert ruDetails.TiKVRUV2() equals the expected fallback value (or remains unchanged) after each call to UpdateTiKVRUV2FromExecDetailsV2 to lock down behavior for nil ExecDetailsV2 and nil RuV2 cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tikvrpc/tikvrpc.go`:
- Around line 1349-1359: BatchCopStreamResponse.Recv currently only clears the
deadline and CountRPC flag but does not perform the RPC/accounting work streams
are expected to do; copy the RPC accounting path from sendRequest() into Recv():
when ret != nil (i.e., a real BatchResponse is received) set resp.BatchResponse
= ret, ensure resp.CountRPC is set so this response is charged, and invoke the
same RPC counter/RUv2 update logic used in sendRequest() (the exact bookkeeping
calls used there) so the stream propagates RPC counters and updates RU metrics
for BatchCopStreamResponse.
---
Nitpick comments:
In `@config/ruv2_test.go`:
- Around line 26-55: Add tests that exercise the nil-branch in
UpdateTiKVRUV2FromExecDetailsV2 by calling it with details == nil and with
&kvrpcpb.ExecDetailsV2{RuV2: nil} so the compatibility fallback path is covered;
create a similar test setup as TestUpdateTiKVRUV2FromExecDetailsV2 (use
DefaultConfig(), DefaultRUV2TiKVConfig(), StoreGlobalConfig, util.NewRUDetails()
and ctx with util.RUDetailsCtxKey) and assert ruDetails.TiKVRUV2() equals the
expected fallback value (or remains unchanged) after each call to
UpdateTiKVRUV2FromExecDetailsV2 to lock down behavior for nil ExecDetailsV2 and
nil RuV2 cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 234b9772-a19e-4397-9f7a-af4a313730a5
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
config/ruv2.goconfig/ruv2_test.goexamples/gcworker/go.modexamples/rawkv/go.modexamples/txnkv/1pc_txn/go.modexamples/txnkv/async_commit/go.modexamples/txnkv/delete_range/go.modexamples/txnkv/go.modexamples/txnkv/pessimistic_txn/go.modexamples/txnkv/unsafedestoryrange/go.modgo.modintegration_tests/go.modinternal/client/client.gointernal/client/client_async.gointernal/client/client_async_test.gotikvrpc/tikvrpc.gotikvrpc/tikvrpc_test.go
nolouch
approved these changes
Apr 1, 2026
ekexium
approved these changes
Apr 1, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, nolouch The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
disksing
force-pushed
the
disksing/rpc-interceptor
branch
from
22624f0 to
ec6a7ae
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tikvrpc/tikvrpc.go (1)
1348-1361:⚠️ Potential issue | 🔴 CriticalMissing
UpdateTiKVRUV2FromExecDetailsV2call causes BatchCop RU v2 accounting to be dropped.
BatchCopStreamResponse.Recv()resetsCountRPCbut never callsconfig.UpdateTiKVRUV2FromExecDetailsV2. In contrast,CopStreamResponse.Recv()(lines 1326-1332) correctly implements this.The
Ctxfield is wired up ingetBatchCopStreamResponsebut is never used here. Sinceclient.goexcludesBatchCopStreamResponsefrom the default handler with the comment "Stream responses are handled in Recv()", BatchCop streams will have zero RU v2 accounting.Proposed fix to add RU v2 accounting
func (resp *BatchCopStreamResponse) Recv() (*coprocessor.BatchResponse, error) { deadline := time.Now().Add(resp.Timeout).UnixNano() atomic.StoreInt64(&resp.deadline, deadline) ret, err := resp.Tikv_BatchCoprocessorClient.Recv() atomic.StoreInt64(&resp.deadline, 0) // Stop the lease check. if ret != nil { resp.BatchResponse = ret + readRPCCount := int64(0) + if resp.CountRPC { + resp.CountRPC = false + readRPCCount = 1 + } + config.UpdateTiKVRUV2FromExecDetailsV2(resp.Ctx, ret.GetExecDetailsV2(), readRPCCount, 0) - resp.CountRPC = false } return ret, errors.WithStack(err) }Note: You may also want to add a
Bypassfield toBatchCopStreamResponsefor consistency withCopStreamResponse, or verify that BatchCop requests should never bypass resource control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tikvrpc/tikvrpc.go` around lines 1348 - 1361, BatchCopStreamResponse.Recv() resets CountRPC and clears the deadline but never updates RU v2 accounting; add a call to config.UpdateTiKVRUV2FromExecDetailsV2 using resp.Ctx and the exec details from the received response (similar to CopStreamResponse.Recv()) before clearing CountRPC so BatchCop RU v2 usage is recorded; ensure you read exec details from ret (resp.BatchResponse) when ret != nil, call UpdateTiKVRUV2FromExecDetailsV2(resp.Ctx, execDetails, resp.CountRPC, /*bypass*/ false or a new resp.Bypass field if you add it for parity with CopStreamResponse), then reset resp.CountRPC and deadline as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tikvrpc/tikvrpc.go`:
- Around line 1348-1361: BatchCopStreamResponse.Recv() resets CountRPC and
clears the deadline but never updates RU v2 accounting; add a call to
config.UpdateTiKVRUV2FromExecDetailsV2 using resp.Ctx and the exec details from
the received response (similar to CopStreamResponse.Recv()) before clearing
CountRPC so BatchCop RU v2 usage is recorded; ensure you read exec details from
ret (resp.BatchResponse) when ret != nil, call
UpdateTiKVRUV2FromExecDetailsV2(resp.Ctx, execDetails, resp.CountRPC, /*bypass*/
false or a new resp.Bypass field if you add it for parity with
CopStreamResponse), then reset resp.CountRPC and deadline as currently done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee210a7f-6d65-45dc-a8ca-6be2431c2868
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
config/ruv2.goconfig/ruv2_test.goexamples/gcworker/go.modexamples/rawkv/go.modexamples/txnkv/1pc_txn/go.modexamples/txnkv/async_commit/go.modexamples/txnkv/delete_range/go.modexamples/txnkv/go.modexamples/txnkv/pessimistic_txn/go.modexamples/txnkv/unsafedestoryrange/go.modgo.modintegration_tests/go.modinternal/client/client.gointernal/client/client_async.gointernal/client/client_async_test.gotikvrpc/tikvrpc.gotikvrpc/tikvrpc_test.go
✅ Files skipped from review due to trivial changes (8)
- examples/txnkv/unsafedestoryrange/go.mod
- integration_tests/go.mod
- examples/txnkv/delete_range/go.mod
- examples/rawkv/go.mod
- examples/gcworker/go.mod
- examples/txnkv/1pc_txn/go.mod
- examples/txnkv/go.mod
- examples/txnkv/async_commit/go.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/txnkv/pessimistic_txn/go.mod
- internal/client/client_async.go
- internal/client/client_async_test.go
- go.mod
- tikvrpc/tikvrpc_test.go
- config/ruv2_test.go
- config/ruv2.go
13 tasks
This was referenced Apr 10, 2026
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
This PR updates the statement RUv2 raw RPC counters to flow through
ExecDetailsV2.RuV2instead of extendingRUDetailsin client-go.Specifically:
github.com/pingcap/kvprototo the latest master that includesRUV2.read_rpc_countandRUV2.write_rpc_countRUDetailsExecDetailsV2.RuV2.ReadRpcCountandExecDetailsV2.RuV2.WriteRpcCountin client-go when handling completed RPCsExecDetailsV2.RuV2, with a fallback for responses that still do not carryExecDetailsV2Why
The previous approach stored extra raw counters in
RUDetailsonly so TiDB could read them back later. Now that kvproto exposes these counters inExecDetailsV2.RuV2, client-go can put them directly into the protocol detail object and avoid carrying an additional side channel inRUDetails.This keeps the raw counter source aligned with other RUv2 fields and simplifies the client-go runtime detail structure.
Impact
ExecDetailsV2.RuV2RUDetailsextension added only for this featureExecDetailsV2Tests
go test ./util ./config ./internal/client ./tikvrpcSummary by CodeRabbit
Bug Fixes
Chores
Tests