util, config: aggregate raw RUv2 in RUDetails#1941
Conversation
ti-chi-bot
bot
added
the
dco-signoff: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR refactors TiKV RU v2 tracking by introducing a new exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@txnkv/txnsnapshot/snapshot.go`:
- Around line 450-451: The code currently skips calling
updateTiKVRUV2FromReadResponse when ExecDetailsV2 (variable details) is nil, so
ReadRpcCount metrics from responses without exec details are not aggregated;
change the logic to always call updateTiKVRUV2FromReadResponse(ctx, details)
without gating on details != nil (the function already handles details == nil),
and apply the same removal of the nil guard at the other two occurrences that
gate on ExecDetailsV2 (the blocks around where details is checked and where the
callers at the two other sites perform the same nil check) so snapshot reads
always contribute ReadRpcCount aggregation even when ExecDetailsV2 is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da28ce29-95da-4186-8a8c-d9f3615b1c56
📒 Files selected for processing (2)
txnkv/txnsnapshot/snapshot.gotxnkv/txnsnapshot/snapshot_async.go
txnkv/txnsnapshot/snapshot.go
Outdated
| if details != nil { | ||
| updateTiKVRUV2FromReadResponse(ctx, details) |
There was a problem hiding this comment.
Count read RPCs even when ExecDetailsV2 is absent.
config.UpdateTiKVRUV2FromExecDetailsV2 already supports details == nil, but Line 467 and both callers still gate the backfill on a non-nil ExecDetailsV2. That means snapshot reads still miss raw ReadRpcCount aggregation on responses that omit exec details, which is the exact gap this change is trying to close.
💡 Proposed fix
@@
- if details != nil {
- updateTiKVRUV2FromReadResponse(ctx, details)
+ updateTiKVRUV2FromReadResponse(ctx, details)
+ if details != nil {
readKeys := len(pairs)
var readTime float64
@@
func updateTiKVRUV2FromReadResponse(ctx context.Context, details *kvrpcpb.ExecDetailsV2) {
- if details == nil || details.GetRuV2().GetReadRpcCount() != 0 {
+ if details != nil && details.GetRuV2().GetReadRpcCount() != 0 {
return
}
config.UpdateTiKVRUV2FromExecDetailsV2(ctx, details, 1, 0)
}
@@
- if cmdGetResp.ExecDetailsV2 != nil {
- updateTiKVRUV2FromReadResponse(bo.GetCtx(), cmdGetResp.ExecDetailsV2)
+ updateTiKVRUV2FromReadResponse(bo.GetCtx(), cmdGetResp.ExecDetailsV2)
+ if cmdGetResp.ExecDetailsV2 != nil {
readKeys := len(cmdGetResp.Value)
var readTime float64Also applies to: 466-470, 888-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@txnkv/txnsnapshot/snapshot.go` around lines 450 - 451, The code currently
skips calling updateTiKVRUV2FromReadResponse when ExecDetailsV2 (variable
details) is nil, so ReadRpcCount metrics from responses without exec details are
not aggregated; change the logic to always call
updateTiKVRUV2FromReadResponse(ctx, details) without gating on details != nil
(the function already handles details == nil), and apply the same removal of the
nil guard at the other two occurrences that gate on ExecDetailsV2 (the blocks
around where details is checked and where the callers at the two other sites
perform the same nil check) so snapshot reads always contribute ReadRpcCount
aggregation even when ExecDetailsV2 is absent.
…entally Accumulate raw TiKV RU v2 counters (kvrpcpb.RUV2) in RUDetails so that TiDB can drain them incrementally via DrainRUV2 without relying on RPC interceptors. - Add RUDetails.AddRUV2 / RUV2 / DrainRUV2 for accumulation and drain - Synthesize RUV2 containers when response lacks ExecDetailsV2 - Backfill ReadRpcCount for get/batchGet when TiKV omits it - Sync commit-path WriteRUV2 into RUDetails after Commit - Extract UpdateTiKVRUV2FromRUV2 and calculateTiKVRUV2 helpers Signed-off-by: disksing <i@disksing.com>
Move updateTiKVRUV2FromReadResponse calls outside the ExecDetailsV2 nil guards so snapshot reads always contribute ReadRpcCount backfill. Remove the redundant nil check inside updateTiKVRUV2FromReadResponse since the downstream UpdateTiKVRUV2FromExecDetailsV2 already handles nil details. Signed-off-by: disksing <i@disksing.com>
…entally on response Instead of aggregating write RUV2 into CommitDetails and draining it after commit, call config.UpdateTiKVRUV2FromExecDetailsV2 directly in the prewrite and commit response handlers. This makes write RUV2 collection consistent with the read path's incremental approach. Signed-off-by: disksing <i@disksing.com>
disksing
force-pushed
the
disksing/ru-details-ruv2
branch
from
f8f7bf9 to
38f53ee
Compare
ti-chi-bot
bot
added
size/XL
1 participant
Summary
util.RUDetailsExecDetailsV2.RuV2intoRUDetailswhile keeping the existing scaled TiKV RUv2 accumulationWhy
TiDB needs to collect RUv2 metrics from multiple request paths, including coprocessor, point/batch reads, and 2PC writes. Aggregating the raw RUv2 counters in
RUDetailsgives TiDB one unified place to read them from, instead of having to read fromExecDetailsV2or commit details separately on each path.Test
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes