session,store: remove RUv2 rpc interceptor (#67508)#68019
session,store: remove RUv2 rpc interceptor (#67508)#68019ti-chi-bot[bot] merged 5 commits intopingcap:release-nextgen-202603from
Conversation
ti-chi-bot
Bot
added
release-note-none
|
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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the per-statement RUv2 RPC interceptor and its tests; adds execdetails snapshot and RUv2 sync helpers; propagates/synchronizes RUv2 into statement-level metrics across executor, resultset, and session; switches ExecDetails consumers to snapshot reads; and updates Go/Bazel dependency pins. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant TiKV as TiKV (ExecDetailsV2)
participant ExecDetails as execdetails
participant Session as Session/DistSQL
participant Executor as Executor/RecordSet
Client->>TiKV: RPCs produce ExecDetailsV2 (with RUV2)
TiKV-->>Client: Responses carrying ExecDetailsV2
Client->>ExecDetails: LoadTiKVExecDetails(pointer) -> snapshot
ExecDetails-->>Session: provide snapshot RUDetails / RUV2Metrics
Session->>Executor: ctx (with RUV2Metrics / RUDetails)
Executor->>Executor: SyncRUV2MetricsFromContext -> sessVars.RUV2Metrics
Executor->>Session: report/accumulate RU values (no per-RPC interceptor)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (1)
pkg/util/stmtsummary/v2/record.go (1)
435-444: Redundant atomic loads via doubleLoadTiKVExecDetails.
StmtNetworkTrafficSummary.Add(seepkg/util/stmtsummary/statement_summary.goLine 1119-1131) internally callsexecdetails.LoadTiKVExecDetails(info)on its argument. Passing&tikvExecDetailshere causes an additional 8 atomic loads against the already-snapshotted fields. It's semantically fine (the snapshot's fields are plain int64, andatomic.LoadInt64on them returns the same values), but wasteful on a hot path.Minor — consider either passing
info.TiKVExecDetailsdirectly and lettingAddproduce the single snapshot, or providing an overload that accepts the pre-loaded snapshot by value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/stmtsummary/v2/record.go` around lines 435 - 444, You are double-loading TiKV exec details: the code calls execdetails.LoadTiKVExecDetails(info.TiKVExecDetails) into tikvExecDetails and then passes &tikvExecDetails to StmtNetworkTrafficSummary.Add which itself calls execdetails.LoadTiKVExecDetails again; to fix, change the call to pass the original info.TiKVExecDetails (not &tikvExecDetails) into StmtNetworkTrafficSummary.Add so Add performs the single atomic snapshot via execdetails.LoadTiKVExecDetails(info.TiKVExecDetails); alternatively, if you want to keep a preloaded snapshot, add an Add overload that accepts the preloaded execdetails.TiKVExecDetails by value and use that.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 128: go.mod pins github.com/tikv/pd/client to
v0.0.0-20260404141330-8a6813497b52 but DEPS.bzl's com_github_tikv_pd_client
entry still references the older v0.0.0-20260401072359-048f0d8f6f71; run make
bazel_prepare to refresh DEPS.bzl and update the com_github_tikv_pd_client block
so its strip_prefix, URLs and sha256 match the new
v0.0.0-20260404141330-8a6813497b52 version (commit/timestamp) committed to the
repo.
In `@pkg/util/execdetails/ruv2_metrics.go`:
- Around line 120-125: SyncRUV2MetricsFromRUDetails currently returns early when
metrics.Bypass() is true and never calls ruDetails.DrainRUV2(), which leaves the
RUv2 delta buffered and may leak into subsequent statements; fix by invoking
ruDetails.DrainRUV2() unconditionally at the start of
SyncRUV2MetricsFromRUDetails to consume/clear the buffered delta, then if
metrics.Bypass() return without calling UpdateRUV2MetricsFromRUV2; keep
UpdateRUV2MetricsFromRUV2(metrics, ...) only for the non-bypassed path so the
raw delta is always drained but only applied when appropriate.
---
Nitpick comments:
In `@pkg/util/stmtsummary/v2/record.go`:
- Around line 435-444: You are double-loading TiKV exec details: the code calls
execdetails.LoadTiKVExecDetails(info.TiKVExecDetails) into tikvExecDetails and
then passes &tikvExecDetails to StmtNetworkTrafficSummary.Add which itself calls
execdetails.LoadTiKVExecDetails again; to fix, change the call to pass the
original info.TiKVExecDetails (not &tikvExecDetails) into
StmtNetworkTrafficSummary.Add so Add performs the single atomic snapshot via
execdetails.LoadTiKVExecDetails(info.TiKVExecDetails); alternatively, if you
want to keep a preloaded snapshot, add an Add overload that accepts the
preloaded execdetails.TiKVExecDetails by value and use that.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b220df8a-be45-45d4-b470-4d7a42c46066
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
DEPS.bzlgo.modpkg/ddl/backfilling_operators.gopkg/distsql/context/BUILD.bazelpkg/distsql/context/context.gopkg/distsql/context/context_test.gopkg/distsql/distsql.gopkg/executor/BUILD.bazelpkg/executor/adapter.gopkg/executor/adapter_test.gopkg/executor/explain.gopkg/executor/explain_unit_test.gopkg/executor/staticrecordset/BUILD.bazelpkg/executor/staticrecordset/recordset.gopkg/server/BUILD.bazelpkg/server/conn_stmt_test.gopkg/server/internal/resultset/resultset.gopkg/session/BUILD.bazelpkg/session/session.gopkg/session/tidb_test.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/tests/slowlog/slow_log_test.gopkg/sessiontxn/isolation/BUILD.bazelpkg/sessiontxn/isolation/base.gopkg/sessiontxn/isolation/base_test.gopkg/sessiontxn/staleread/BUILD.bazelpkg/sessiontxn/staleread/provider.gopkg/store/copr/BUILD.bazelpkg/store/copr/coprocessor.gopkg/store/copr/ruv2_metrics.gopkg/store/driver/txn/BUILD.bazelpkg/store/driver/txn/ruv2_metrics.gopkg/store/driver/txn/ruv2_metrics_test.gopkg/util/execdetails/BUILD.bazelpkg/util/execdetails/execdetails_test.gopkg/util/execdetails/ruv2_metrics.gopkg/util/execdetails/tiflash_stats.gopkg/util/execdetails/util.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/v2/record.go
💤 Files with no reviewable changes (13)
- pkg/session/BUILD.bazel
- pkg/sessiontxn/staleread/BUILD.bazel
- pkg/store/copr/BUILD.bazel
- pkg/distsql/context/BUILD.bazel
- pkg/distsql/context/context.go
- pkg/distsql/distsql.go
- pkg/sessiontxn/staleread/provider.go
- pkg/store/driver/txn/ruv2_metrics.go
- pkg/store/driver/txn/ruv2_metrics_test.go
- pkg/store/copr/ruv2_metrics.go
- pkg/sessiontxn/isolation/base.go
- pkg/store/copr/coprocessor.go
- pkg/sessiontxn/isolation/base_test.go
| github.com/tikv/client-go/v2 v2.0.8-0.20260401083018-b7f9a9e9d2ab | ||
| github.com/tikv/pd/client v0.0.0-20260401072359-048f0d8f6f71 | ||
| github.com/tikv/client-go/v2 v2.0.8-0.20260414033830-1adc54c38a51 | ||
| github.com/tikv/pd/client v0.0.0-20260404141330-8a6813497b52 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm DEPS.bzl pd/client strip_prefix vs go.mod version.
rg -n 'com_github_tikv_pd_client|tikv/pd/client' DEPS.bzl | head -40
echo '---'
rg -n 'tikv/pd/client' go.modRepository: pingcap/tidb
Length of output: 920
DEPS.bzl pd/client entry is out of sync with this bump.
go.mod now pins github.com/tikv/pd/client to v0.0.0-20260404141330-8a6813497b52, but the corresponding DEPS.bzl entry (com_github_tikv_pd_client) still references v0.0.0-20260401072359-048f0d8f6f71 (different timestamp 20260401 vs 20260404 and different commit hash). Bazel builds and go builds will resolve to different pd/client versions, which can cause subtle behavioral drift and CI failures.
Rerun make bazel_prepare and commit the refreshed DEPS.bzl (strip_prefix + URLs + sha256 for pd/client).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 128, go.mod pins github.com/tikv/pd/client to
v0.0.0-20260404141330-8a6813497b52 but DEPS.bzl's com_github_tikv_pd_client
entry still references the older v0.0.0-20260401072359-048f0d8f6f71; run make
bazel_prepare to refresh DEPS.bzl and update the com_github_tikv_pd_client block
so its strip_prefix, URLs and sha256 match the new
v0.0.0-20260404141330-8a6813497b52 version (commit/timestamp) committed to the
repo.
| func SyncRUV2MetricsFromRUDetails(metrics *RUV2Metrics, ruDetails *tikvutil.RUDetails) { | ||
| if metrics == nil || ruDetails == nil || metrics.Bypass() { | ||
| return | ||
| } | ||
| UpdateRUV2MetricsFromRUV2(metrics, ruDetails.DrainRUV2()) | ||
| } |
There was a problem hiding this comment.
Drain RUDetails even when statement RUv2 is bypassed.
When metrics.Bypass() is true this returns before ruDetails.DrainRUV2(), so the raw RUv2 delta stays buffered in the shared RUDetails. With the new ContextWithMissingExecDetailsInitialized flow reusing existing exec-detail objects on a reused context, the skipped delta can leak into the next non-bypassed statement and get mis-attributed there.
Proposed fix
func SyncRUV2MetricsFromRUDetails(metrics *RUV2Metrics, ruDetails *tikvutil.RUDetails) {
- if metrics == nil || ruDetails == nil || metrics.Bypass() {
+ if ruDetails == nil {
return
}
- UpdateRUV2MetricsFromRUV2(metrics, ruDetails.DrainRUV2())
+ drained := ruDetails.DrainRUV2()
+ if metrics == nil || metrics.Bypass() {
+ return
+ }
+ UpdateRUV2MetricsFromRUV2(metrics, drained)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func SyncRUV2MetricsFromRUDetails(metrics *RUV2Metrics, ruDetails *tikvutil.RUDetails) { | |
| if metrics == nil || ruDetails == nil || metrics.Bypass() { | |
| return | |
| } | |
| UpdateRUV2MetricsFromRUV2(metrics, ruDetails.DrainRUV2()) | |
| } | |
| func SyncRUV2MetricsFromRUDetails(metrics *RUV2Metrics, ruDetails *tikvutil.RUDetails) { | |
| if ruDetails == nil { | |
| return | |
| } | |
| drained := ruDetails.DrainRUV2() | |
| if metrics == nil || metrics.Bypass() { | |
| return | |
| } | |
| UpdateRUV2MetricsFromRUV2(metrics, drained) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/execdetails/ruv2_metrics.go` around lines 120 - 125,
SyncRUV2MetricsFromRUDetails currently returns early when metrics.Bypass() is
true and never calls ruDetails.DrainRUV2(), which leaves the RUv2 delta buffered
and may leak into subsequent statements; fix by invoking ruDetails.DrainRUV2()
unconditionally at the start of SyncRUV2MetricsFromRUDetails to consume/clear
the buffered delta, then if metrics.Bypass() return without calling
UpdateRUV2MetricsFromRUV2; keep UpdateRUV2MetricsFromRUV2(metrics, ...) only for
the non-bypassed path so the raw delta is always drained but only applied when
appropriate.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #68019 +/- ##
===========================================================
Coverage ? 77.5581%
===========================================================
Files ? 1962
Lines ? 543904
Branches ? 0
===========================================================
Hits ? 421842
Misses ? 121209
Partials ? 853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, disksing, wjhuang2016, XuHuaiyu 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 |
ti-chi-bot
Bot
merged commit 7961967
into
pingcap:release-nextgen-202603
6 participants
ref #67199
cp #67508
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
New Features
Tests
Chores