kv: support transactional delete mutations#239
Open
lance6716 wants to merge 2 commits into
Open
Conversation
ti-chi-bot
Bot
added
the
dco-signoff: yes
📝 WalkthroughWalkthroughThis PR adds transaction mutation objects that represent Put and Delete operations, threads them through the transaction buffer and 2PC commit path, adds delete coverage in tests, and makes the CI test script wait for ChangesTransaction delete semantics
CI test startup readiness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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/M
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for delete operations in transactions by introducing a TxnMutation type that carries both the operation type (Put/Del) and value, replacing the previous string-only buffer.
Changes:
- Introduced
TxnMutationstruct encapsulating op type and value - Added
Txn::del()API and updatedget()/2PC prewrite to handle deletes - Added a new test exercising txn delete
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| include/pingcap/kv/2pc.h | Defines TxnMutation; changes mutations map value type; uses valueSize() for size calc |
| include/pingcap/kv/Txn.h | Buffer now maps to TxnMutation; adds del(); updates get() and walkBuffer() signature |
| src/kv/2pc.cc | Propagates op type into prewrite request; skips value for Del ops |
| src/test/region_split_test.cc | Adds testTxnDelete covering set/get/del/get flow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+47
| void set(const std::string & key, const std::string & value) { buffer.emplace(key, TxnMutation::Put(value)); } | ||
|
|
||
| void del(const std::string & key) { buffer.emplace(key, TxnMutation::Del()); } |
Comment on lines
+98
to
+102
| { | ||
| Snapshot snap(test_cluster.get(), test_cluster->pd_client->getTS()); | ||
| ASSERT_EQ(snap.Get("delete_test_key"), ""); | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/region_split_test.cc (1)
79-102: ⚡ Quick winAdd same-transaction mutation-order regression cases.
This test validates cross-transaction delete, but not
set->delanddel->setwithin a single txn, which are key buffer semantics forTxnMutation.✅ Suggested test additions
TEST_F(TestWithMockKVRegionSplit, testTxnDelete) { @@ { Snapshot snap(test_cluster.get(), test_cluster->pd_client->getTS()); ASSERT_EQ(snap.Get("delete_test_key"), ""); } + + // set then delete in the same txn => key should not exist + { + Txn txn(test_cluster.get()); + txn.set("delete_test_key_2", "v1"); + txn.del("delete_test_key_2"); + txn.commit(); + } + { + Snapshot snap(test_cluster.get(), test_cluster->pd_client->getTS()); + ASSERT_EQ(snap.Get("delete_test_key_2"), ""); + } + + // delete then set in the same txn => latest mutation should win + { + Txn txn(test_cluster.get()); + txn.del("delete_test_key_3"); + txn.set("delete_test_key_3", "v2"); + txn.commit(); + } + { + Snapshot snap(test_cluster.get(), test_cluster->pd_client->getTS()); + ASSERT_EQ(snap.Get("delete_test_key_3"), "v2"); + } }🤖 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 `@src/test/region_split_test.cc` around lines 79 - 102, Extend the testTxnDelete in TestWithMockKVRegionSplit to cover same-transaction mutation-order regressions: add one case that performs txn.set("key","val") then txn.del("key") within the same Txn (followed by txn.commit()) and assert via Snapshot.Get("key") that the key is absent, and add the complementary case that does txn.del("key") then txn.set("key","val") in the same Txn (commit) and assert Snapshot.Get("key") returns the new value; use the existing Txn, Snapshot, txn.set, txn.del, txn.commit and snap.Get patterns to implement these two blocks alongside the existing cross-transaction checks.
🤖 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 `@include/pingcap/kv/Txn.h`:
- Around line 45-47: Txn::set and Txn::del currently use buffer.emplace(...)
which ignores subsequent mutations for the same key; change these to overwrite
semantics so later set/del replace prior entries by using map::insert_or_assign
(or operator[]) when writing into Txn::buffer, and similarly replace emplace
calls in TwoPhaseCommitter's walkBuffer-to-2PC-mutation logic with
insert_or_assign so the last mutation for a key wins; update references to
TxnMutation construction (TxnMutation::Put/Del) accordingly.
---
Nitpick comments:
In `@src/test/region_split_test.cc`:
- Around line 79-102: Extend the testTxnDelete in TestWithMockKVRegionSplit to
cover same-transaction mutation-order regressions: add one case that performs
txn.set("key","val") then txn.del("key") within the same Txn (followed by
txn.commit()) and assert via Snapshot.Get("key") that the key is absent, and add
the complementary case that does txn.del("key") then txn.set("key","val") in the
same Txn (commit) and assert Snapshot.Get("key") returns the new value; use the
existing Txn, Snapshot, txn.set, txn.del, txn.commit and snap.Get patterns to
implement these two blocks alongside the existing cross-transaction checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f838173-5cb1-4ce5-9f59-90b5e3eb0f01
📒 Files selected for processing (4)
include/pingcap/kv/2pc.hinclude/pingcap/kv/Txn.hsrc/kv/2pc.ccsrc/test/region_split_test.cc
lance6716
force-pushed
the
lance/txn-delete-support
branch
from
06a5e0b to
37b3a4f
Compare
ti-chi-bot
Bot
added
size/L
Signed-off-by: lance6716 <lance6716@gmail.com>
lance6716
force-pushed
the
lance/txn-delete-support
branch
from
37b3a4f to
76845ad
Compare
gengliqi
approved these changes
Jun 30, 2026
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/region_split_test.cc (1)
106-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct
del→delsame-key case.This test covers
set→delanddel→set, but not two consecutivedel()calls on the same key. Since delete mutations now carry no value, that exact overwrite path is worth pinning down in this suite.Possible addition
{ Txn txn(test_cluster.get()); txn.set("delete_test_key_2", "v1"); txn.del("delete_test_key_2"); + txn.del("delete_test_key_2"); auto buffered_result = txn.get("delete_test_key_2"); ASSERT_FALSE(buffered_result.second); ASSERT_EQ(buffered_result.first, ""); txn.commit(); }🤖 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 `@src/test/region_split_test.cc` around lines 106 - 129, Add a direct same-key delete-over-delete case to the transaction test in region_split_test.cc, alongside the existing buffered mutation checks in the Txn/Snapshot flow. Extend the test around the current delete/set scenarios to create a Txn, call del() twice on the same key, verify txn.get() reflects a pending delete after the second call, and commit to confirm the overwrite path behaves correctly. Use the existing Txn and Snapshot patterns in this test to keep the coverage consistent.
🤖 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 `@ci/build-test.sh`:
- Around line 32-34: The mock-tikv shutdown in build-test.sh is conditional on
make test succeeding, so cleanup can be skipped when the script exits early
under set -e. Move the kill of $mock_kv_pid into an EXIT trap immediately after
the mock-tikv process is started, and keep the existing test run in the main
flow so the cleanup always runs regardless of success or failure.
---
Nitpick comments:
In `@src/test/region_split_test.cc`:
- Around line 106-129: Add a direct same-key delete-over-delete case to the
transaction test in region_split_test.cc, alongside the existing buffered
mutation checks in the Txn/Snapshot flow. Extend the test around the current
delete/set scenarios to create a Txn, call del() twice on the same key, verify
txn.get() reflects a pending delete after the second call, and commit to confirm
the overwrite path behaves correctly. Use the existing Txn and Snapshot patterns
in this test to keep the coverage consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f694a5be-bbd4-4791-988e-b1f6d4804685
📒 Files selected for processing (5)
ci/build-test.shinclude/pingcap/kv/2pc.hinclude/pingcap/kv/Txn.hsrc/kv/2pc.ccsrc/test/region_split_test.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- src/kv/2pc.cc
- include/pingcap/kv/2pc.h
- include/pingcap/kv/Txn.h
Comment on lines
32
to
34
| cd "$build_dir" && make test | ||
|
|
||
| kill -9 $mock_kv_pid |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect cleanup flow in ci/build-test.sh"
sed -n '1,80p' ci/build-test.sh
echo
echo "Search CI scripts for trap-based background-process cleanup patterns"
rg -n 'trap .*EXIT|kill -9 .*mock|nohup .*mock-tikv' ciRepository: tikv/client-c
Length of output: 1085
Make mock-tikv cleanup unconditional.
kill -9 $mock_kv_pid only runs if make test succeeds; with set -e, a test failure exits before cleanup, leaving mock-tikv running. Move it into an EXIT trap right after startup.
Suggested fix
nohup /mock-tikv/bin/mock-tikv &
mock_kv_pid=$!
+trap 'kill -9 "$mock_kv_pid" >/dev/null 2>&1 || true' EXIT
@@
-cd "$build_dir" && make test
-
-kill -9 $mock_kv_pid
+cd "$build_dir" && make test🤖 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 `@ci/build-test.sh` around lines 32 - 34, The mock-tikv shutdown in
build-test.sh is conditional on make test succeeding, so cleanup can be skipped
when the script exits early under set -e. Move the kill of $mock_kv_pid into an
EXIT trap immediately after the mock-tikv process is started, and keep the
existing test run in the main flow so the cleanup always runs regardless of
success or failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Authorship
This PR was implemented by Codex, an AI coding agent, under user direction.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores