store: mark TiDB requests for max-ts validation (#68779) - #68960
store: mark TiDB requests for max-ts validation (#68779)#68960ti-chi-bot[bot] merged 16 commits into
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. |
ti-chi-bot
Bot
added
size/M
|
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:
📝 WalkthroughWalkthroughSet TiKV client's default RequestOrigin to TiDB (Bazel wiring and imports added), validate RequestOrigin in session tests, update many Bazel and Go dependency pins, adjust go.mod requires, and increase a test shard count. ChangesRequest Origin Wiring
Dependency pins and module updates
Test shard adjustment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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
🤖 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 `@cmd/tidb-server/main.go`:
- Around line 304-311: Replace the direct deferred call that captures err by
value with a deferred closure so EndStandby sees the final err; change the
current defer standbyController.EndStandby(err) to defer func() {
standbyController.EndStandby(err) }() near the WaitForActivate() call (this
ensures standbyController.EndStandby observes errors from later calls like
registerStores() and prepareKeyspaceObservabilityForStarter()).
- Around line 284-289: The startup config validation currently exits with
os.Exit(0) on invalid configs in the kerneltype.IsNextGen() and
kerneltype.IsClassic() branches; change both exits to a non-zero status (e.g.,
os.Exit(1)) so failures are reported to systemd/Kubernetes/scripts; update the
two occurrences where os.Exit(0) is called in the block that checks
kerneltype.IsNextGen(), kerneltype.IsClassic(), and config.GetGlobalConfig()
(including Standby.StandByMode, KeyspaceName, KeyspaceActivateMode) to use
os.Exit(1).
🪄 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: fa4dc002-a50f-40ff-b327-624b4bb85235
📒 Files selected for processing (3)
cmd/tidb-server/BUILD.bazelcmd/tidb-server/main.gopkg/session/test/session_test.go
| if kerneltype.IsNextGen() && len(config.GetGlobalConfig().KeyspaceName) == 0 && !config.GetGlobalConfig().Standby.StandByMode { | ||
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name or standby mode is required for nextgen TiDB") | ||
| os.Exit(0) | ||
| } else if kerneltype.IsClassic() && (len(config.GetGlobalConfig().KeyspaceName) > 0 || config.GetGlobalConfig().Standby.StandByMode || config.GetGlobalConfig().KeyspaceActivateMode) { | ||
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name, standby mode or keyspace-activate mode is not supported for classic TiDB") | ||
| os.Exit(0) |
There was a problem hiding this comment.
Return a non-zero status for invalid startup config.
Lines 285-289 reject invalid config but exit with os.Exit(0). That reports success to systemd/Kubernetes/scripts, so a misconfigured TiDB can look like a clean startup/shutdown instead of a failed boot.
Suggested fix
if kerneltype.IsNextGen() && len(config.GetGlobalConfig().KeyspaceName) == 0 && !config.GetGlobalConfig().Standby.StandByMode {
fmt.Fprintln(os.Stderr, "invalid config: keyspace name or standby mode is required for nextgen TiDB")
- os.Exit(0)
+ os.Exit(1)
} else if kerneltype.IsClassic() && (len(config.GetGlobalConfig().KeyspaceName) > 0 || config.GetGlobalConfig().Standby.StandByMode || config.GetGlobalConfig().KeyspaceActivateMode) {
fmt.Fprintln(os.Stderr, "invalid config: keyspace name, standby mode or keyspace-activate mode is not supported for classic TiDB")
- os.Exit(0)
+ os.Exit(1)
}📝 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.
| if kerneltype.IsNextGen() && len(config.GetGlobalConfig().KeyspaceName) == 0 && !config.GetGlobalConfig().Standby.StandByMode { | |
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name or standby mode is required for nextgen TiDB") | |
| os.Exit(0) | |
| } else if kerneltype.IsClassic() && (len(config.GetGlobalConfig().KeyspaceName) > 0 || config.GetGlobalConfig().Standby.StandByMode || config.GetGlobalConfig().KeyspaceActivateMode) { | |
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name, standby mode or keyspace-activate mode is not supported for classic TiDB") | |
| os.Exit(0) | |
| if kerneltype.IsNextGen() && len(config.GetGlobalConfig().KeyspaceName) == 0 && !config.GetGlobalConfig().Standby.StandByMode { | |
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name or standby mode is required for nextgen TiDB") | |
| os.Exit(1) | |
| } else if kerneltype.IsClassic() && (len(config.GetGlobalConfig().KeyspaceName) > 0 || config.GetGlobalConfig().Standby.StandByMode || config.GetGlobalConfig().KeyspaceActivateMode) { | |
| fmt.Fprintln(os.Stderr, "invalid config: keyspace name, standby mode or keyspace-activate mode is not supported for classic TiDB") | |
| os.Exit(1) | |
| } |
🤖 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 `@cmd/tidb-server/main.go` around lines 284 - 289, The startup config
validation currently exits with os.Exit(0) on invalid configs in the
kerneltype.IsNextGen() and kerneltype.IsClassic() branches; change both exits to
a non-zero status (e.g., os.Exit(1)) so failures are reported to
systemd/Kubernetes/scripts; update the two occurrences where os.Exit(0) is
called in the block that checks kerneltype.IsNextGen(), kerneltype.IsClassic(),
and config.GetGlobalConfig() (including Standby.StandByMode, KeyspaceName,
KeyspaceActivateMode) to use os.Exit(1).
| // If running standby mode, wait for activate request. | ||
| if standbyController != nil { | ||
| standbyController.WaitForActivate() | ||
| // EndStandby only execute once. If server is created | ||
| // successfully, the defer has no effect. If panics | ||
| // before server is created, the defer makes sure to | ||
| // notify the activate caller. | ||
| defer standbyController.EndStandby(err) |
There was a problem hiding this comment.
Defer EndStandby via closure so it sees the final error.
Line 311 passes err to defer by value, so it is captured as nil at defer time. If registerStores() or prepareKeyspaceObservabilityForStarter() fails later, EndStandby still receives nil and the activator can be told startup succeeded even though bootstrap aborted.
Suggested fix
- defer standbyController.EndStandby(err)
+ defer func() {
+ standbyController.EndStandby(err)
+ }()📝 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.
| // If running standby mode, wait for activate request. | |
| if standbyController != nil { | |
| standbyController.WaitForActivate() | |
| // EndStandby only execute once. If server is created | |
| // successfully, the defer has no effect. If panics | |
| // before server is created, the defer makes sure to | |
| // notify the activate caller. | |
| defer standbyController.EndStandby(err) | |
| // If running standby mode, wait for activate request. | |
| if standbyController != nil { | |
| standbyController.WaitForActivate() | |
| // EndStandby only execute once. If server is created | |
| // successfully, the defer has no effect. If panics | |
| // before server is created, the defer makes sure to | |
| // notify the activate caller. | |
| defer func() { | |
| standbyController.EndStandby(err) | |
| }() |
🤖 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 `@cmd/tidb-server/main.go` around lines 304 - 311, Replace the direct deferred
call that captures err by value with a deferred closure so EndStandby sees the
final err; change the current defer standbyController.EndStandby(err) to defer
func() { standbyController.EndStandby(err) }() near the WaitForActivate() call
(this ensures standbyController.EndStandby observes errors from later calls like
registerStores() and prepareKeyspaceObservabilityForStarter()).
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ekexium
removed
the
do-not-merge/hold
|
/retest |
Signed-off-by: Ziqian Qin <eke@fastmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68960 +/- ##
================================================
Coverage ? 55.6168%
================================================
Files ? 1847
Lines ? 665365
Branches ? 0
================================================
Hits ? 370055
Misses ? 267741
Partials ? 27569
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
…k-68779-to-release-8.5 Signed-off-by: Ziqian Qin <eke@fastmail.com> # Conflicts: # DEPS.bzl # go.mod # go.sum
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
Bot
added
size/XXL
…68779-to-release-8.5 Signed-off-by: Ziqian Qin <eke@fastmail.com> # Conflicts: # pkg/planner/core/casetest/rule/BUILD.bazel
Signed-off-by: Ziqian Qin <eke@fastmail.com>
|
/retest |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
a78dfe8 to
421a41d
Compare
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest |
close pingcap#65727 (cherry picked from commit 6d55295) Signed-off-by: Ziqian Qin <eke@fastmail.com>
|
/retest |
1 similar comment
|
/retest |
…68779-to-release-8.5
close pingcap#67174 (cherry picked from commit 25eacc4) Signed-off-by: Ziqian Qin <eke@fastmail.com>
…ap#68169) close pingcap#67174 (cherry picked from commit c6056f8) Signed-off-by: Ziqian Qin <eke@fastmail.com>
After the dependency update the test binary's live heap after the 810MB allocation sits a few hundred KB below the runtime's memory-limit heap goal (1GiB*0.8 = 819.2MiB minus non-heap memory and headroom), so the runtime fires no further GC during the wait windows. The memory limit tuner's finalizer hook therefore never gets the second run it needs to set adjustPercentageInProgress, and the waits time out. This used to pass only because the heap margin was small enough that the polling goroutine's own allocations crossed the goal and triggered a GC. Force runtime.GC() in the polling conditions (the same approach as PR pingcap#67377) so detection no longer depends on incidental allocations. ref pingcap#65673 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Ziqian Qin <eke@fastmail.com>
…68779-to-release-8.5
|
/retest |
1 similar comment
|
/retest |
The analyze-after-DDL goroutine reports failures via an unbuffered doneCh. When analyzeTableInner returns through the cumulative-timeout branch, nothing ever receives from doneCh; a later analyze failure blocks the goroutine on the send forever, leaking the ddl session pool session and deadlocking ddl.Stop()/Domain.Close() in SetCapacity(0). This is what made //tests/realtikvtest/addindextest4 (TestAnalyzeTimeout) hang deterministically once testify v1.11 (pulled in by the grpc upgrade) started evaluating require.Eventually conditions immediately. Mirror the master fix in a736b2f (pingcap#66870): make doneCh buffered. ref pingcap#69127 Signed-off-by: Ziqian Qin <eke@fastmail.com>
|
@ti-chi-bot: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, winoros, wjhuang2016 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 |
7 participants
This is an automated cherry-pick of #68779
What problem does this PR solve?
Issue Number: close #68799
Problem Summary:
TiKV needs to distinguish TiDB-originated requests from other client-go users so selected non-TiDB max-ts update paths can be validated without drift tolerance.
What changed and how does it work?
REQUEST_ORIGIN_TIDBfromcmd/tidb-server.TestRequestSourceto verify TiDB requests carrykvrpcpb.Context.request_originon the wire.replacedirectives so this draft can compile before upstream dependency PRs merge.The temporary dependency replaces should be removed after pingcap/kvproto#1479 and tikv/client-go#1975 are merged and TiDB can bump normal upstream module versions.
Check List
Tests
Commands:
make bazel_prepareGOPRIVATE=github.com/ekexium/kvproto,github.com/ekexium/client-go ./tools/check/failpoint-go-test.sh pkg/session/test -run TestRequestSourceGOPRIVATE=github.com/ekexium/kvproto,github.com/ekexium/client-go gotestsum --format short-verbose -- ./cmd/tidb-server -run '^$'GOPRIVATE=github.com/ekexium/kvproto,github.com/ekexium/client-go make lintgit diff --checkSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Chores
Tests