session,store: remove RUv2 rpc interceptor#67508
session,store: remove RUv2 rpc interceptor#67508disksing wants to merge 1 commit intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughRemoved per-statement tikv Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant DistSQL
participant TxnDriver as Txn/StoreDriver
participant TiKV
participant Copr as Coprocessor
participant ExecDetails as execdetails
rect rgba(220,220,255,0.5)
Note over Session,TiKV: Old flow (interceptor-based)
Session->>DistSQL: create request (attach statement RPC interceptor)
DistSQL->>TxnDriver: send RPC (wrapped by interceptor)
TxnDriver->>TiKV: RPC call
TiKV-->>TxnDriver: response + ExecDetailsV2
TxnDriver->>TxnDriver: interceptor observes response
TxnDriver->>Session: interceptor updates statement RUV2 metrics
end
rect rgba(220,255,220,0.5)
Note over Session,TiKV: New flow (centralized)
Session->>DistSQL: create request (no statement interceptor)
DistSQL->>TxnDriver: send RPC
TxnDriver->>TiKV: RPC call
TiKV-->>TxnDriver: response + ExecDetailsV2
TxnDriver->>Copr: coprocessor handler receives response
Copr->>ExecDetails: call UpdateRUV2MetricsFromExecDetailsV2(RUV2MetricsFromContext(ctx), ExecDetailsV2)
ExecDetails-->Copr: metrics updated centrally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/XXL
|
Hi @disksing. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
/retest |
|
/ok-to-test |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
/retest |
1 similar comment
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67508 +/- ##
================================================
- Coverage 77.5929% 77.4225% -0.1705%
================================================
Files 1981 1973 -8
Lines 548157 548059 -98
================================================
- Hits 425331 424321 -1010
- Misses 122016 123334 +1318
+ Partials 810 404 -406
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test |
|
@disksing: The The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/test all |
|
@disksing: The Use 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 kubernetes-sigs/prow repository. |
|
/test fast_test_tiprow |
|
@disksing: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
@disksing: The specified target(s) for Use 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 kubernetes-sigs/prow repository. |
|
/retest |
| require.NotEqual(t, baseline, metrics.CalculateRUValues(updated)) | ||
| } | ||
|
|
||
| func TestUpdateRUV2MetricsFromExecDetailsV2(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM] Test coverage gap for end-to-end RUv2 collection.
Could we add a regression test for real statement paths (for example point get / batch get / prewrite) and assert resource_manager_read_cnt and resource_manager_write_cnt are updated? Current tests here validate helper mapping, but they do not verify the end-to-end collection path after interceptor removal.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/session/tidb_test.go (1)
266-285: Isolate setup DDL from the RUv2 assertion session.Using the same session for table setup and verification can let prior write metrics mask regressions in the target INSERT path. A separate setup session keeps this assertion focused and deterministic.
💡 Suggested adjustment
func TestWriteStatementTracksCommitDetailsRUV2Metrics(t *testing.T) { store, dom := CreateStoreAndBootstrap(t) defer func() { require.NoError(t, store.Close()) }() defer dom.Close() - se, err := createSession(store) + setupSe, err := createSession(store) require.NoError(t, err) - MustExec(t, se, "use test") - MustExec(t, se, "drop table if exists write_ruv2") - MustExec(t, se, "create table write_ruv2 (id int primary key, v int)") + MustExec(t, setupSe, "use test") + MustExec(t, setupSe, "drop table if exists write_ruv2") + MustExec(t, setupSe, "create table write_ruv2 (id int primary key, v int)") + + se, err := createSession(store) + require.NoError(t, err) + MustExec(t, se, "use test") stmt, err := se.ParseWithParams(context.Background(), "insert into write_ruv2 values (1, 1)") require.NoError(t, err) _, err = se.ExecuteStmt(context.Background(), stmt) require.NoError(t, err) require.NotNil(t, se.sessionVars.RUV2Metrics) require.Greater(t, se.sessionVars.RUV2Metrics.ResourceManagerWriteCnt(), int64(0)) }As per coding guidelines: "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/tidb_test.go` around lines 266 - 285, The test TestWriteStatementTracksCommitDetailsRUV2Metrics currently uses the same session (se created by createSession) for both DDL setup and the measured INSERT, which can pollute RUV2 metrics; change the test to perform the schema setup (MustExec "use test", "drop table...", "create table...") using a separate setup session (call createSession to get a setupSe and close it after DDL), then create a fresh session (the existing se) only for ParseWithParams/ExecuteStmt and the RUV2 assertion (referencing se.sessionVars.RUV2Metrics and calling se.ParseWithParams / se.ExecuteStmt) so prior writes from setup do not affect ResourceManagerWriteCnt.pkg/session/session.go (1)
989-990: Consider centralizing the post-commit RUv2 sync.
CommitTxnandRefreshTxnCtxnow carry the sameMergeExecDetails+UpdateRUV2MetricsFromCommitDetailssequence. A small helper would keep those two bookkeeping steps in lockstep and make the new no-interceptor flow easier to maintain.As per coding guidelines, code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature.
Also applies to: 4894-4895
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/session.go` around lines 989 - 990, Both CommitTxn and RefreshTxnCtx repeat the same post-commit bookkeeping (s.sessionVars.StmtCtx.MergeExecDetails and execdetails.UpdateRUV2MetricsFromCommitDetails), so extract those two calls into a small helper (e.g., a method on session like s.syncPostCommitExecDetails(commitDetail) or a package-level function SyncRUV2FromCommit(sessionVars, commitDetail)) and replace the duplicated sequences in CommitTxn and RefreshTxnCtx (and at the other occurrence around lines 4894-4895) with a single call to that helper to keep the bookkeeping centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/session/session.go`:
- Around line 989-990: Both CommitTxn and RefreshTxnCtx repeat the same
post-commit bookkeeping (s.sessionVars.StmtCtx.MergeExecDetails and
execdetails.UpdateRUV2MetricsFromCommitDetails), so extract those two calls into
a small helper (e.g., a method on session like
s.syncPostCommitExecDetails(commitDetail) or a package-level function
SyncRUV2FromCommit(sessionVars, commitDetail)) and replace the duplicated
sequences in CommitTxn and RefreshTxnCtx (and at the other occurrence around
lines 4894-4895) with a single call to that helper to keep the bookkeeping
centralized and consistent.
In `@pkg/session/tidb_test.go`:
- Around line 266-285: The test TestWriteStatementTracksCommitDetailsRUV2Metrics
currently uses the same session (se created by createSession) for both DDL setup
and the measured INSERT, which can pollute RUV2 metrics; change the test to
perform the schema setup (MustExec "use test", "drop table...", "create
table...") using a separate setup session (call createSession to get a setupSe
and close it after DDL), then create a fresh session (the existing se) only for
ParseWithParams/ExecuteStmt and the RUV2 assertion (referencing
se.sessionVars.RUV2Metrics and calling se.ParseWithParams / se.ExecuteStmt) so
prior writes from setup do not affect ResourceManagerWriteCnt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d186bc2f-304b-4cf3-b447-82cf300247af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
DEPS.bzlgo.modpkg/session/session.gopkg/session/tidb_test.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/ruv2_metrics.gopkg/util/stmtsummary/BUILD.bazelpkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/record.go
✅ Files skipped from review due to trivial changes (2)
- pkg/util/stmtsummary/BUILD.bazel
- pkg/util/stmtsummary/v2/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/util/execdetails/ruv2_metrics.go
- go.mod
|
/retest |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
retest |
disksing
force-pushed
the
rpc-interceptor
branch
from
6a559e1 to
f3bf62b
Compare
|
[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 |
Signed-off-by: disksing <i@disksing.com>
disksing
force-pushed
the
rpc-interceptor
branch
from
0af607a to
1ab1572
Compare
|
@disksing: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@disksing: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: ref #67199
Problem Summary:
TiDB currently relies on a dedicated RUv2 RPC interceptor to collect statement-level raw RPC counters. After kvproto exposes these counters on
ExecDetailsV2.RuV2and client-go starts filling them there, TiDB can remove the extra interceptor path and read the data directly from exec details.What changed and how does it work?
github.com/pingcap/kvprototo the latest master, including pingcap/kvproto#1445github.com/tikv/client-go/v2to the latest master, including tikv/client-go#1933ExecDetailsV2.RuV2intoRUV2MetricsCheck List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit