txnkv: support to SetCommitWaitUntilTSOTimeout and do not wait TSO#1876
txnkv: support to SetCommitWaitUntilTSOTimeout and do not wait TSO#1876ti-chi-bot[bot] merged 2 commits intotikv:masterfrom
SetCommitWaitUntilTSOTimeout and do not wait TSO#1876Conversation
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis change modifies the CommitWaitUntilTSO timeout handling in both tests and the transaction implementation. When the timeout is set to zero, the system now immediately returns an error instead of falling back to a default behavior. The test infrastructure is updated to exercise this no-wait scenario. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes touch a critical commit path with modified timeout semantics and new error handling, but are localized to two well-scoped files with relatively few lines modified. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 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)
373-377:⚠️ Potential issue | 🟡 MinorStale doc comment: zero now means "no wait", not "use default".
The comment on
GetCommitWaitUntilTSOTimeoutsays zero means "it is not set and the default timeout should be used," but with this PR, zero now triggers an immediateErrCommitTSLag. This should be updated to reflect the new semantics.📝 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 +// If it returns zero, the commit will fail immediately with ErrCommitTSLag when the PD TSO lags behind the expected value. func (txn *KVTxn) GetCommitWaitUntilTSOTimeout() time.Duration { return txn.commitWaitUntilTSOTimeout }
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 |
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