txnkv: add readPoolTaskDetails to SnapshotRuntimeStats - #2025
Conversation
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe change adds read-pool task metric aggregation and exposes it through snapshot runtime statistics. It also updates Go 1.25.10 and refreshes kvproto, PD client, gRPC, protobuf, and genproto versions across modules. ChangesRead-pool statistics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExecDetailsV2
participant mergeExecDetail
participant SnapshotRuntimeStats
ExecDetailsV2->>mergeExecDetail: provide readPoolTaskDetails
mergeExecDetail->>SnapshotRuntimeStats: merge read-pool metrics
SnapshotRuntimeStats->>SnapshotRuntimeStats: clone, merge, and format details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)examples/gcworker/go.modTraceback (most recent call last): examples/rawkv/go.modTraceback (most recent call last): examples/txnkv/1pc_txn/go.modTraceback (most recent call last):
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/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/gcworker/go.mod`:
- Line 3: Update the Go version from 1.25.10 to 1.25.12 in the module directives
at examples/gcworker/go.mod:3-3, examples/rawkv/go.mod:3-3,
examples/txnkv/1pc_txn/go.mod:3-3, examples/txnkv/async_commit/go.mod:3-3,
examples/txnkv/delete_range/go.mod:3-3, examples/txnkv/go.mod:3-3,
examples/txnkv/pessimistic_txn/go.mod:3-3, and
examples/txnkv/unsafedestoryrange/go.mod:3-3; if retaining 1.25.10 instead,
document the intentional pin at every affected module.
In `@txnkv/txnsnapshot/snapshot.go`:
- Around line 951-956: Add focused _test.go coverage in txnkv/txnsnapshot for
mergeExecDetail’s read-pool task details path, SnapshotRuntimeStats.Merge and
Clone behavior, String output, and GetReadPoolTaskDetails. Verify
initialization, merging, cloning independence, and expected formatted output
using the existing symbols and test conventions.
🪄 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: 6da3f9eb-4d5b-4d36-b5bb-8804ec55d5ec
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
examples/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.modtxnkv/txnsnapshot/snapshot.goutil/execdetails.goutil/execdetails_test.go
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
8863e20 to
e4c4f91
Compare
| hadTaskWallTimeSamples := d.TaskWallTimeSampleCount > 0 | ||
| hadTasks := d.TaskCount > 0 | ||
|
|
||
| d.TaskCount++ |
There was a problem hiding this comment.
The PoolTaskDetails is a cumulative snapshot of one physical read-pool task, but batch_get_command can attach successive snapshots of the same shared task to multiple child responses.
Incrementing the TaskCount and summing each snapshot here therefore double-counts task/poll/time totals and may attribute earlier children’s work to later requests.
There was a problem hiding this comment.
This aggregation is intended for request-latency diagnostics rather than de-duplicated physical task accounting. Each response contributes one task observation. For a child response from batch_get_command, the cumulative snapshot intentionally includes work performed before that child becomes visible, because that work contributes to its observed latency. Totals are sums across reported observations and are primarily used to calculate averages; overlapping or shared physical read-pool tasks are not de-duplicated. We will clarify this behavior in the field and merge documentation.
We could alternatively report statistics based on distinct physical tasks by de-duplicating shared execution on the TiKV side. However, doing so would require substantially more changes, such as introducing task identity and snapshot semantics across kvproto, TiKV, and client-go, and retaining that identity during aggregation. We need to evaluate whether the additional complexity is justified. In practice, a single SQL statement only encounters this distinction in a limited set of cases, such as multiple UNION branches that each perform a point get.
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, zyguan 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 |
|
/cherry-pick release-nextgen-202603 |
|
@lcwangchao: new pull request created to branch DetailsIn response to this:
Instructions 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 ti-community-infra/tichi repository. |
ref tidbcloud/cloud-storage-engine#5480 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ref tidbcloud/cloud-storage-engine#5480 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
ref tidbcloud/cloud-storage-engine#5480\n\nSigned-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>\nSigned-off-by: Chao Wang <cclcwangchao@hotmail.com>\n\nCo-authored-by: Chao Wang <cclcwangchao@hotmail.com>
4 participants
What problem does this PR solve?
Issue Number: ref https://github.com/tidbcloud/cloud-storage-engine/issues/5480
Problem Summary:
TiKV/CSE can now return optional request-level read-pool scheduling and execution metrics in
ExecDetailsV2.read_pool_task_details. client-go currently drops these details, so TiDB and other downstream consumers cannot include read-pool queueing, wakeup, dispatch/poll, and CPU/wall-time breakdowns in snapshot runtime diagnostics.What is changed and how it works?
PoolTaskDetailstoExecDetailsV2.util.PoolTaskDetailsto aggregate reported task samples, including:read_pool_task_detailsfrom responses intoSnapshotRuntimeStats.GetReadPoolTaskDetails()as a defensive copy so callers cannot mutate the stored runtime stats.The aggregation operates on reported response samples because the protobuf currently carries no task identity or delta information.
Check List
go test ./util ./txnkv/txnsnapshotgo test --tags=intest ./util ./txnkv/txnsnapshotread_pool_task_detailsis absent.Related changes
Summary by CodeRabbit