vars: validate some vars for tidb x#68196
Conversation
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
Bot
added
the
release-note-none
|
@ekexium I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
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 (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR bumps the tikv client-go/v2 dependency, adds Validation hooks to disallow non-leader TiDBReplicaRead modes and the ChangesNextGen Feature Restrictions
Sequence Diagram(s)sequenceDiagram
participant User as Client/API
participant Sysvar as sysvar.TiDBReplicaRead/TiDBDMLType
participant Kernel as kerneltype.IsNextGen
User->>Sysvar: SET replica_read / SET dml_type
Sysvar->>Kernel: IsNextGen()
Kernel-->>Sysvar: true/false
Sysvar-->>User: accept or ErrNotSupportedInNextGen
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds NextGen (TiDB X) restrictions to session/system variables so that features incompatible with the NextGen kernel cannot be enabled via sysvars, aligning runtime behavior with the NextGen feature set.
Changes:
- Add NextGen-only validation for
tidb_replica_readto reject any mode other thanleader. - Add NextGen-only validation for
tidb_dml_typeto rejectbulkmode. - Update and add unit tests (including
//go:build nextgencoverage) to assert the new validation behavior. - Bump
github.com/tikv/client-go/v2dependency and BazelDEPS.bzlmetadata accordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sessionctx/variable/sysvar.go | Adds NextGen validation hooks for tidb_replica_read and tidb_dml_type. |
| pkg/sessionctx/variable/varsutil_test.go | Updates session var tests to expect NextGen validation failures for replica read follower/mixed modes. |
| pkg/sessionctx/variable/sysvar_test.go | Updates sysvar validation test to branch on NextGen vs Classic behavior for tidb_replica_read. |
| pkg/sessionctx/variable/nextgen_test.go | Adds NextGen-only tests verifying tidb_dml_type and tidb_replica_read are constrained as intended. |
| go.mod | Bumps github.com/tikv/client-go/v2 pseudo-version. |
| go.sum | Updates checksums for the bumped client-go version. |
| DEPS.bzl | Updates Bazel go_repository strip_prefix/sha256/urls for the new client-go version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68196 +/- ##
================================================
- Coverage 77.7288% 76.8860% -0.8429%
================================================
Files 1990 1973 -17
Lines 551970 554613 +2643
================================================
- Hits 429040 426420 -2620
- Misses 122010 128096 +6086
+ Partials 920 97 -823
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 NOT APPROVED This pull-request has been approved by: cfzjywxk 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 |
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
release-note
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/test/distsqltest/distsql_test.go (1)
133-140: ⚡ Quick winConsider adding a comment explaining the next-gen restriction.
The conditional logic correctly restricts replica read modes when running on next-gen kernels, aligning with the PR objective. However, future maintainers would benefit from a brief comment explaining why non-leader modes are excluded for next-gen (e.g., "Next-gen TiKV does not support non-leader replica reads").
📝 Suggested comment
replicaReadModes := []string{ "leader", } + // Next-gen kernel does not support non-leader replica read modes. if !kerneltype.IsNextGen() { replicaReadModes = append(replicaReadModes,🤖 Prompt for 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. In `@pkg/executor/test/distsqltest/distsql_test.go` around lines 133 - 140, Add a brief explanatory comment above the kerneltype.IsNextGen() conditional to clarify why non-leader replica read modes are omitted for next-gen kernels (e.g., that Next-gen TiKV does not support non-leader replica reads or that replica reads are handled differently), so future maintainers understand the restriction; locate the check using kerneltype.IsNextGen() and the replicaReadModes slice to add the comment.
🤖 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.
Nitpick comments:
In `@pkg/executor/test/distsqltest/distsql_test.go`:
- Around line 133-140: Add a brief explanatory comment above the
kerneltype.IsNextGen() conditional to clarify why non-leader replica read modes
are omitted for next-gen kernels (e.g., that Next-gen TiKV does not support
non-leader replica reads or that replica reads are handled differently), so
future maintainers understand the restriction; locate the check using
kerneltype.IsNextGen() and the replicaReadModes slice to add the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fcb2f961-7d58-431a-8039-1ff36edc755a
📒 Files selected for processing (1)
pkg/executor/test/distsqltest/distsql_test.go
ekexium
force-pushed
the
validate-tidbx-vars
branch
from
4cbed3c to
f005b8d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/executor_failpoint_test.go (1)
810-818: 💤 Low valueCorrect conditional gating for NextGen replica-read restrictions.
The stale-read test is correctly skipped on NextGen kernels where non-leader
tidb_replica_readmodes (like'closest-replicas') are disabled per the PR objectives.Consider adding a brief comment above line 810 explaining why this test is skipped on NextGen for future maintainability:
// Skip stale read with replica read test on NextGen where non-leader replica reads are disabled. if !kerneltype.IsNextGen() {🤖 Prompt for 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. In `@pkg/executor/executor_failpoint_test.go` around lines 810 - 818, Add a one-line comment explaining why the stale-read replica test is skipped when NextGen is enabled: place it immediately above the existing if !kerneltype.IsNextGen() check in executor_failpoint_test.go, referencing that NextGen disables non-leader replica reads (so modes like 'closest-replicas' are not applicable). Keep the comment short and specific (e.g., "Skip stale read with replica read test on NextGen where non-leader replica reads are disabled."), leaving the surrounding code calling waitUntilReadTSSafe and the explain/require assertions unchanged.
🤖 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.
Nitpick comments:
In `@pkg/executor/executor_failpoint_test.go`:
- Around line 810-818: Add a one-line comment explaining why the stale-read
replica test is skipped when NextGen is enabled: place it immediately above the
existing if !kerneltype.IsNextGen() check in executor_failpoint_test.go,
referencing that NextGen disables non-leader replica reads (so modes like
'closest-replicas' are not applicable). Keep the comment short and specific
(e.g., "Skip stale read with replica read test on NextGen where non-leader
replica reads are disabled."), leaving the surrounding code calling
waitUntilReadTSSafe and the explain/require assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e2adce6-3c60-4473-badf-d3318f23a554
📒 Files selected for processing (6)
pkg/executor/distsql_test.gopkg/executor/executor_failpoint_test.gopkg/executor/test/distsqltest/distsql_test.gopkg/session/test/variable/variable_test.gopkg/sessiontxn/isolation/main_test.gopkg/sessiontxn/staleread/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/executor/test/distsqltest/distsql_test.go
ekexium
force-pushed
the
validate-tidbx-vars
branch
from
f005b8d to
86bfa7b
Compare
|
/retest |
|
@ekexium: 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. |
What problem does this PR solve?
Issue Number: close #60697
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
New Features
Dependencies
Tests