vars: validate some vars for tidb x (#68196)#68458
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@ekexium This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughThis PR enforces next-gen kernel restrictions: updates TiKV client pins, adds validation to ChangesNextGen Feature Gating: Replica Read and DML Type Restrictions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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/sessionctx/variable/varsutil_test.go (1)
384-393: ⚡ Quick winAssert post-error replica-read state in next-gen branches.
These branches validate the error, but they don’t verify that session state remains
"leader"after rejected assignments. Adding that check prevents false positives where an error is returned but state is still mutated.Proposed test hardening
err = v.SetSystemVar(vardef.TiDBReplicaRead, "follower") if kerneltype.IsNextGen() { require.Error(t, err) require.True(t, ErrNotSupportedInNextGen.Equal(err)) + val, err = v.GetSessionOrGlobalSystemVar(context.Background(), vardef.TiDBReplicaRead) + require.NoError(t, err) + require.Equal(t, "leader", val) + require.Equal(t, kv.ReplicaReadLeader, v.replicaRead) } else { require.NoError(t, err) val, err = v.GetSessionOrGlobalSystemVar(context.Background(), vardef.TiDBReplicaRead) require.NoError(t, err) require.Equal(t, "follower", val) require.Equal(t, kv.ReplicaReadFollower, v.replicaRead) } @@ err = v.SetSystemVar(vardef.TiDBReplicaRead, "leader-and-follower") if kerneltype.IsNextGen() { require.Error(t, err) require.True(t, ErrNotSupportedInNextGen.Equal(err)) + val, err = v.GetSessionOrGlobalSystemVar(context.Background(), vardef.TiDBReplicaRead) + require.NoError(t, err) + require.Equal(t, "leader", val) + require.Equal(t, kv.ReplicaReadLeader, v.replicaRead) } else { require.NoError(t, err) val, err = v.GetSessionOrGlobalSystemVar(context.Background(), vardef.TiDBReplicaRead) require.NoError(t, err) require.Equal(t, "leader-and-follower", val) require.Equal(t, kv.ReplicaReadMixed, v.replicaRead) }Also applies to: 401-410
🤖 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/sessionctx/variable/varsutil_test.go` around lines 384 - 393, The test currently only asserts an error in the kerneltype.IsNextGen() branch but does not verify that the session state was not mutated; update the failing-branch assertions to also read back the TiDBReplicaRead state and assert it is still "leader" and that v.replicaRead == kv.ReplicaReadLeader (use v.GetSessionOrGlobalSystemVar(ctx, vardef.TiDBReplicaRead) and check v.replicaRead) so both the error and unchanged session state are verified; make the same addition in the other similar branch around lines 401-410 to harden that assertion too.
🤖 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 `@DEPS.bzl`:
- Around line 7808-7824: Remove the leftover merge-conflict markers in DEPS.bzl
and leave a single valid dependency block for github.com/tikv/client-go/v2:
delete the lines starting with <<<<<<<, =======, and >>>>>>> and ensure only one
set of sha256, strip_prefix, and urls remains (pick the correct/expected values
for sha256, strip_prefix, and urls for the chosen revision) so the file parses
cleanly for Bazel.
In `@go.mod`:
- Around line 127-133: go.mod contains leftover git conflict markers (<<<<<<<,
=======, >>>>>>>) that break parsing; open the go.mod and remove the conflict
markers and duplicate entries, pick the correct version for
github.com/tikv/client-go/v2 and github.com/tikv/pd/client (or reconcile to the
desired versions used elsewhere), leaving only one line per module (e.g., a
single chosen version for github.com/tikv/client-go/v2 and for
github.com/tikv/pd/client) and run go mod tidy to verify the file is valid.
---
Nitpick comments:
In `@pkg/sessionctx/variable/varsutil_test.go`:
- Around line 384-393: The test currently only asserts an error in the
kerneltype.IsNextGen() branch but does not verify that the session state was not
mutated; update the failing-branch assertions to also read back the
TiDBReplicaRead state and assert it is still "leader" and that v.replicaRead ==
kv.ReplicaReadLeader (use v.GetSessionOrGlobalSystemVar(ctx,
vardef.TiDBReplicaRead) and check v.replicaRead) so both the error and unchanged
session state are verified; make the same addition in the other similar branch
around lines 401-410 to harden that assertion too.
🪄 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: 2fe53fe6-5492-4864-9d4e-3d3c3157d764
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
DEPS.bzlgo.modpkg/executor/distsql_test.gopkg/executor/executor_failpoint_test.gopkg/executor/test/distsqltest/BUILD.bazelpkg/executor/test/distsqltest/distsql_test.gopkg/session/test/variable/BUILD.bazelpkg/session/test/variable/variable_test.gopkg/sessionctx/variable/nextgen_test.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/sysvar_test.gopkg/sessionctx/variable/varsutil_test.gopkg/sessiontxn/isolation/main_test.gopkg/sessiontxn/staleread/BUILD.bazelpkg/sessiontxn/staleread/provider_test.gotests/realtikvtest/sessiontest/session_fail_test.gotests/realtikvtest/txntest/replica_read_test.gotests/realtikvtest/txntest/stale_read_test.go
Signed-off-by: Ziqian Qin <eke@fastmail.com>
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #68458 +/- ##
===========================================================
Coverage ? 77.0918%
===========================================================
Files ? 1966
Lines ? 548272
Branches ? 0
===========================================================
Hits ? 422673
Misses ? 124747
Partials ? 852
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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, yudongusa, 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 |
|
/retest |
|
/test pull-integration-realcluster-test-next-gen |
|
@wuhuizuo: No presubmit jobs available for pingcap/tidb@release-nextgen-202603 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. |
ti-chi-bot
Bot
merged commit 6e625df
into
pingcap:release-nextgen-202603
6 participants
This is an automated cherry-pick of #68196
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
Bug Fixes
Chores