txnkv: support to SetCommitWaitUntilTSOTimeout and do not wait TSO#1875
txnkv: support to SetCommitWaitUntilTSOTimeout and do not wait TSO#1875ti-chi-bot[bot] merged 2 commits intotikv:tidb-8.5from
SetCommitWaitUntilTSOTimeout and do not wait TSO#1875Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThe changes modify timeout handling in the commit-wait-until-TSO flow. When the timeout is zero, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
4bd2d6d to
0b84a25
Compare
|
/retest |
|
@coderabbitai review this PR again |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
txnkv/transaction/txn.go (1)
320-324:⚠️ Potential issue | 🟡 MinorStale doc comment: zero now means "no wait", not "use default".
The comment on
GetCommitWaitUntilTSOTimeoutsays zero means "not set and the default timeout should be used," but with this PR, zero now means immediate failure. This will confuse future readers.Suggested fix
-// GetCommitWaitUntilTSOTimeout returns the timeout to wait for the commit tso reach the expected value. -// If it returns zero, it means it is not set and the default timeout should be used +// GetCommitWaitUntilTSOTimeout returns the timeout to wait for the commit TSO to reach the expected value. +// If it returns zero, the commit will fail immediately with ErrCommitTSLag when PD TSO lags.
🧹 Nitpick comments (1)
txnkv/transaction/txn.go (1)
1886-1917: Duplicate comment block and clean logic for the zero-timeout path look correct.Two issues in this segment:
The comment block at lines 1886–1888 is duplicated verbatim at lines 1907–1909. The first occurrence (1886–1888) is now orphaned — it no longer sits adjacent to the
maxSleepassignment. Please remove one of the two.The zero-timeout fast-fail logic (lines 1910–1917) is correct and aligns with the PR objective.
Remove the duplicate comment block
- // maxSleep is the maximum time we are allowed to wait for the expected commit TS. - // It is 1 second by default to avoid infinite blocking but can be overridden by commitWaitUntilTSOTimeout. - // If the TSO drift is larger than the maxSleep, return error directly. start := time.Now()
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, ekexium 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 |
Summary
close #1854
This PR adds explicit no-wait behavior for commit-wait TSO checks.
When
SetCommitWaitUntilTSO(...)is enabled, users can now setSetCommitWaitUntilTSOTimeout(0)to fail immediately if PD TSO is behind the expected commit-wait timestamp, instead of waiting.Motivation
In active-active replication scenarios, some callers prefer a fast-fail path rather than waiting for TSO catch-up. A zero timeout should represent this intent clearly.
Behavior Change
commitWaitUntilTSOTimeout > 0: keep existing bounded-wait behavior.commitWaitUntilTSOTimeout == 0: returnErrCommitTSLagimmediately on the first lag check.1s.Tests
Added integration coverage in
TestSetCommitWaitUntilTSOfor a new"no wait"case:0;Summary by CodeRabbit
Bug Fixes
Tests