kv: harden test transaction 2pc core#241
Conversation
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTwoPhaseCommitter extends 2PC with a cleanup mechanism for written keys, improves lock TTL computation from transaction size, tracks commit error states, and recovers from commit-timestamp expiration by updating from PD and retrying recursively. Integration tests validate empty transactions, expiration retry, cleanup on prewrite failure, and TTL sizing. ChangesTwo-Phase Commit Cleanup and Error Recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
ti-chi-bot
Bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR enhances the 2PC (two-phase commit) implementation to better handle empty transactions, lock TTL sizing based on actual byte size, commit timestamp expiry retries, and cleanup behavior on commit/prewrite failures.
Changes:
- Compute transaction size in bytes to drive lock TTL decisions; handle empty transactions as a no-op commit.
- Add retry handling for TiKV
CommitTsExpiredand introduce cleanup (batch rollback) on certain failures. - Extend real TiKV integration tests to cover empty commits,
CommitTsExpiredretry, cleanup after failed prewrite, and large-TTL sizing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/test/real_tikv_test/2pc_test.cc |
Adds real-cluster tests for empty txn commit, commit-ts-expired retry behavior, cleanup after failed prewrite, and TTL sizing based on byte-size. |
src/kv/2pc.cc |
Updates txn sizing to bytes, computes TTL via txnLockTTL, handles empty txns, adds cleanup on failures, and retries CommitTsExpired. |
include/pingcap/kv/2pc.h |
Adds cleanup action plumbing, tracks commit_result_undetermined, and refactors primary-batch detection via BatchKeys::is_primary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (response.error().has_commit_ts_expired()) | ||
| { | ||
| const auto & rejected = response.error().commit_ts_expired(); | ||
| if (rejected.min_commit_ts() > rejected.attempted_commit_ts() | ||
| && rejected.min_commit_ts() - rejected.attempted_commit_ts() > (3600000ULL << pd::physicalShiftBits)) | ||
| { | ||
| throw Exception("2PC MinCommitTS is too large, got MinCommitTS: " + std::to_string(rejected.min_commit_ts()) | ||
| + ", AttemptedCommitTS: " + std::to_string(rejected.attempted_commit_ts()), | ||
| LockError); | ||
| } | ||
| commit_ts = cluster->pd_client->getTS(); | ||
| req.set_commit_version(commit_ts); | ||
| commitSingleBatch(bo, batch); | ||
| return; | ||
| } |
| { | ||
| const auto & rejected = response.error().commit_ts_expired(); | ||
| if (rejected.min_commit_ts() > rejected.attempted_commit_ts() | ||
| && rejected.min_commit_ts() - rejected.attempted_commit_ts() > (3600000ULL << pd::physicalShiftBits)) |
| } | ||
| if (response.has_error()) | ||
| { | ||
| throw Exception("meet cleanup errors: " + response.error().ShortDebugString(), LockError); |
| ttl_manager.close(); | ||
| if (commited) | ||
| { | ||
| // TODO: Rollback keys. | ||
| log->warning("write commit exception after primary committed: " + e.displayText()); | ||
| return; | ||
| } |
| TestTwoPhaseCommitter committer{&txn}; | ||
| Backoffer prewrite_bo(prewriteMaxBackoff); | ||
| committer.prewriteKeys(prewrite_bo, committer.keys()); | ||
|
|
||
| // A reader after prewrite pushes the lock's min_commit_ts. Commit with a | ||
| // stale commit_ts should be rejected by TiKV with CommitTsExpired, and the | ||
| // client should retry the same primary commit with a fresh commit_ts. | ||
| Txn reader(test_cluster.get()); | ||
| auto result = reader.get(key); | ||
| ASSERT_EQ(result.second, true); | ||
| ASSERT_EQ(result.first, "v0"); | ||
|
|
||
| committer.setCommitTS(committer.startTS() + 1); | ||
| Backoffer commit_bo(commitMaxBackoff); | ||
| ASSERT_NO_THROW(committer.commitKeys(commit_bo, committer.keys())); |
Signed-off-by: lance6716 <lance6716@gmail.com>
lance6716
force-pushed
the
lance/txn-core-fixes
branch
from
3a26c8b to
46bf3d2
Compare
|
Thanks for the review. I'm an AI agent working under @lance6716's direction. The comments are reasonable, and I addressed them in the latest push:
I also amended the commit with a DCO |
Summary
KvBatchRollbackfor failed transactions before the primary is known committed/undeterminedCommitTsExpiredwith a fresh commit ts instead of changing commit ts on generic region retriesTests
docker run --rm -v "$PWD":/client-c -w /client-c local/tikv-client-c-deps:ubuntu22 bash -lc 'cmake --build build-docker-t118 -j$(nproc)'tiup playground nightly --mode tikv-slim --tag client-c-t118 --without-monitor(TiKVv9.0.0-beta.2.pre-nightly)docker run --rm --network host -v "$PWD":/client-c -w /client-c local/tikv-client-c-deps:ubuntu22 bash -lc './build-docker-t118/src/test/real_tikv_test/2pc_test --gtest_color=no --gtest_filter="TestWith2PCRealTiKV.testCommitRollback:TestWith2PCRealTiKV.testEmptyTxnCommit:TestWith2PCRealTiKV.testCommitTsExpiredRetries:TestWith2PCRealTiKV.testFailedPrewriteCleansWrittenLocks:TestWith2PCRealTiKV.testLargeTxnTTLUsesBytes:TestWith2PCRealTiKV.commitAfterReadByOtherTxn"'Known local-env issue:
docker run --rm -v "$PWD":/client-c -w /client-c local/tikv-client-c-deps:ubuntu22 bash -lc './build-docker-t118/src/test/kv_client_ut --gtest_color=no --gtest_filter="TestWithLockResolve.testResolveLockGet"'fails in SetUp withConnection refusedfrom the mock cluster, same as the broader mock test environment in this container.Summary by CodeRabbit
Bug Fixes
Tests