transaction: Support file based transaction - #1998
Conversation
Signed-off-by: Ping Yu <yuping@pingcap.com>
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughIntroduces transaction-file execution for large transactions through chunked HTTP uploads, region-aware prewrite, commit and rollback, lock resolution, optional region pre-splitting, configuration, metrics, 2PC integration, and unit and integration tests. ChangesTxn File 2PC Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client as KVTxn
participant Committer as twoPhaseCommitter
participant ChunkWriter as chunkWriterClient
participant RegionCache as RegionCache
participant TiKV as TiKV Region
Client->>Committer: Commit()
Committer->>Committer: buildTxnFiles(mutations)
Committer->>ChunkWriter: POST chunk data with CRC32
ChunkWriter-->>Committer: chunk IDs and ranges
Committer->>RegionCache: group chunk ranges into region batches
Committer->>TiKV: Prewrite chunk IDs and TxnSize
TiKV-->>Committer: Prewrite response
Committer->>TiKV: Commit or BatchRollback
TiKV-->>Committer: Commit or rollback response
Committer-->>Client: commit result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metrics/metrics.go (1)
1120-1215:⚠️ Potential issue | 🟠 MajorRegister txn-file metrics in
RegisterMetrics.
metrics/metrics.godefinesTiKVTxnFileRequestCounter,TiKVTxnFileWriteBytes,TiKVTxnFileMutationSizeHistogram, andTiKVTxnFileDuration, butRegisterMetricshas noprometheus.MustRegister(...)calls for any of them, so they won’t be exposed to Prometheus.🔧 Proposed fix
func RegisterMetrics() { @@ prometheus.MustRegister(TiKVTxnLagCommitTSAttemptHistogram) prometheus.MustRegister(TiKVStaleBucketFromPDCounter) + prometheus.MustRegister(TiKVTxnFileRequestCounter) + prometheus.MustRegister(TiKVTxnFileWriteBytes) + prometheus.MustRegister(TiKVTxnFileMutationSizeHistogram) + prometheus.MustRegister(TiKVTxnFileDuration) }🤖 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 `@metrics/metrics.go` around lines 1120 - 1215, RegisterMetrics is missing registrations for the txn-file metrics; add prometheus.MustRegister calls for TiKVTxnFileRequestCounter, TiKVTxnFileWriteBytes, TiKVTxnFileMutationSizeHistogram, and TiKVTxnFileDuration inside the RegisterMetrics function so those metrics are exposed to Prometheus (use the same pattern as the other metrics already registered).
🧹 Nitpick comments (3)
integration_tests/txn_file_test.go (1)
259-268: ⚡ Quick winConsider replacing
time.Sleepwith deterministic synchronization.The 3-second sleep adds latency to CI and may be flaky if the commit goroutine doesn't reach the paused failpoint within that window. A more robust approach would use a channel or another failpoint to signal when the commit has reached the desired state before triggering the split.
For example, you could add a "reached prewrite" failpoint that signals a channel, then wait on that channel before calling
cluster.Split.🤖 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 `@integration_tests/txn_file_test.go` around lines 259 - 268, Replace the non-deterministic time.Sleep with a synchronization channel signaled by a new failpoint: instrument the commit path with a "reached prewrite" failpoint that calls close(prewriteReachedCh) (or sends on it) when the commit goroutine reaches the paused state, then in the test wait on that channel before calling cluster.Split(...) instead of sleeping; keep the existing goroutine that runs txn.Commit(ctx), ensure you still disable the "tikvclient/invalidCacheAndRetry" failpoint after split, and preserve the final <-done wait to ensure Commit finishes. Use identifiers txn.Commit, cluster.Split, failpoint.Disable and the done channel to locate and update the test and add the new failpoint signal in the commit execution path.txnkv/transaction/txn_file_test.go (1)
51-56: ⚡ Quick winUse a local seeded RNG for deterministic test data.
Using package-level
math/randhere can couple outcomes to global RNG state from other tests. Prefer a local RNG with a fixed seed.♻️ Suggested change
func TestChunkSliceSortAndDedup(t *testing.T) { assert := assert.New(t) + rng := rand.New(rand.NewSource(1)) genRndChunkIDs := func() []uint64 { - n := rand.Intn(10) + n := rng.Intn(10) ids := make([]uint64, 0, n) for i := 0; i < n; i++ { - ids = append(ids, uint64(rand.Intn(n+n/2+1))) + ids = append(ids, uint64(rng.Intn(n+n/2+1))) } return ids }🤖 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 `@txnkv/transaction/txn_file_test.go` around lines 51 - 56, The test's genRndChunkIDs uses package-level math/rand causing non-determinism; replace it with a local RNG created via rand.New(rand.NewSource(<fixedSeed>)) (e.g., 42) and use that RNG (r.Intn) for n and id generation inside genRndChunkIDs so the test data is deterministic and not affected by global RNG state; ensure the seed is fixed and deterministic for the test run and that all rand.* calls in genRndChunkIDs are switched to the local RNG variable.txnkv/transaction/2pc.go (1)
343-343: ⚡ Quick winDocument or unexport
MutationsHasDataInRange.This new exported symbol lacks a Go doc comment. If it’s intended to be public, add a doc comment; otherwise make it unexported to keep package surface tight.
As per coding guidelines, "
{tikv,rawkv,txnkv,kv,tikvrpc,oracle,config,metrics,trace,error}/**/*.go: Exported identifiers must have clear Go doc comments when they are part of the public client API".🤖 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 `@txnkv/transaction/2pc.go` at line 343, MutationsHasDataInRange is an exported function without a Go doc comment; either add a clear Go doc comment describing its purpose, parameters (mutations, start, end), return values (firstDataKey, found bool) and behavior, or make it unexported by renaming it to mutationsHasDataInRange if it is not part of the public API; update all call sites accordingly (look for MutationsHasDataInRange references) so the package surface adheres to the exported-identifiers documentation rule.Source: Coding guidelines
🤖 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 `@txnkv/transaction/2pc.go`:
- Around line 356-371: MutationsHasDataInRange currently can return (nil, true)
when the loop never assigns firstDataKey (e.g., range contains only non-write
ops); change the logic in the MutationsHasDataInRange function around the
isInRange(pos) block so you only return true when a valid key was found: inside
the loop set firstDataKey = mutations.GetKey(pos) when pos==0 or
isOpForWrite(mutations.GetOp(pos)), break, and after the loop check if
firstDataKey != nil before returning (firstDataKey, true); if firstDataKey is
nil, return (nil, false) instead. Ensure you reference the existing helpers/vars
(isInRange, mutations.GetOp, mutations.GetKey, isOpForWrite, pos, firstDataKey)
so the change is localized to that block.
In `@txnkv/transaction/txn_file.go`:
- Around line 960-967: Log call is using the wrong error variable: change
logutil.Logger(bo.GetCtx()).Error(..., zap.Error(err)) to pass the
channel-result error (r.err) instead of the outer err; update the occurrences
inside the resultCh receive loop (where r := <-resultCh is handled) so the error
logged and the wrapped error return use r.err together with buildChunkErrMsg
(same fix needed for both occurrences around the resultCh handling).
- Around line 1274-1298: The defer resp.Body.Close() inside the retry loop leaks
resources; instead, close resp.Body immediately on each iteration after you're
done with it. Update the loop around w.cli.Do(req) so that you call
resp.Body.Close() (not defer) after reading the body or before any
continue/return paths (both the non-200 branch where you read bodyStr and the
success branch that reads data via io.ReadAll), ensuring every resp is closed on
that iteration; use the existing variables resp, data, err and the same
retry/backoff logic (retry.BoTiKVRPC, retry.BoTiKVServerBusy, bo.Backoff) but
remove the deferred close.
- Around line 736-762: The dispatch loop can skip spawning goroutines when
rateLimiter.GetToken(exitCh) signals exit, causing the receiver to block waiting
for len(batches) responses; fix by tracking only the number of goroutines
actually launched and closing the result channel when they finish. Replace the
manual receive-count approach with a sync.WaitGroup: before launching a
goroutine call wg.Add(1), inside the goroutine defer wg.Done(), and after the
dispatch loop start a goroutine that does wg.Wait() then close(ch); change the
consumer to range over ch until closed (instead of for i := 0; i < len(batches);
i++), keeping existing uses of rateLimiter.GetToken, rateLimiter.PutToken,
executeTxnFileSliceSingleBatch, regionErrChunks.appendSlice and respecting
returnEarly behavior when reading results.
- Line 332: The condition uses locate.IsFakeRegionError which doesn't exist;
replace that call with retry.IsFakeRegionError(regionErr) so the check reads: if
regionErr.GetEpochNotMatch() == nil || retry.IsFakeRegionError(regionErr) {;
update the reference where this appears (e.g., around the
regionErr/GetEpochNotMatch check in txn_file.go) to use the imported retry
package and its IsFakeRegionError function.
- Around line 889-912: The async goroutine is using the forked backoffer `bo`
that is cancelled by the parent function's deferred `cancel()` (from
`bo.Fork()`), causing `executeTxnFileSliceWithRetry` to fail; to fix, create an
independent backoffer/context for the async path (e.g., construct `asyncBO` from
a non-cancelled context such as `context.Background()` or the store's long-lived
context) and pass that `asyncBO` into the async call inside the `store.Go`
closure instead of the parent `bo`; ensure the async goroutine cleans up/cancels
`asyncBO` when it finishes if needed and keep all references to
`executeTxnFileSliceWithRetry`, `executeTxnFileAction`, `bo.Fork()`, and
`store.Go` so the change is localized.
- Around line 259-263: The call site in MutationsHasDataInRange (in txn_file.go)
uses missing helpers util.GetMaxStartKey and util.GetMinEndKey; add these two
functions to the util package (e.g., func GetMaxStartKey(a, b []byte) []byte and
func GetMinEndKey(a, b []byte) []byte) that return the lexicographically greater
start key and the lexicographically smaller end key respectively (preserving
nil/empty semantics used elsewhere), or replace the calls with existing util
equivalents if they already exist; update imports and any unit tests to
reference the new util functions so MutationsHasDataInRange compiles.
- Around line 1117-1122: The call to resourcecontrol.NewRequestInfo in
txnkv/transaction/txn_file.go fails because only
resourcecontrol.MakeRequestInfo(req *tikvrpc.Request) exists and RequestInfo
fields are unexported; fix by either constructing a *tikvrpc.Request with the
appropriate payload/metadata and replacing NewRequestInfo(...) with
resourcecontrol.MakeRequestInfo(req) in the txn_file code, or (if you prefer API
addition) add an exported constructor function in
internal/resourcecontrol/resource_control.go (e.g., NewRequestInfoFromParams)
that accepts requestSize, accessType, replicaID/leaderStoreID and bypass and
returns a *resourcecontrol.RequestInfo with all internal fields (requestSize,
accessType, bypass, etc.) correctly initialized, then call that new constructor
from txn_file.go.
In `@txnkv/transaction/txn.go`:
- Around line 793-795: The exported method DisableTxnFile on type KVTxn lacks a
Go doc comment; add a one-line Go-style doc comment immediately above the
DisableTxnFile declaration explaining its purpose and behavior (e.g.,
"DisableTxnFile disables writing/using the transaction file for this KVTxn.") so
it complies with the public API doc rule; ensure the comment starts with
"DisableTxnFile" and briefly states that it sets disableTxnFile to true for the
KVTxn instance.
---
Outside diff comments:
In `@metrics/metrics.go`:
- Around line 1120-1215: RegisterMetrics is missing registrations for the
txn-file metrics; add prometheus.MustRegister calls for
TiKVTxnFileRequestCounter, TiKVTxnFileWriteBytes,
TiKVTxnFileMutationSizeHistogram, and TiKVTxnFileDuration inside the
RegisterMetrics function so those metrics are exposed to Prometheus (use the
same pattern as the other metrics already registered).
---
Nitpick comments:
In `@integration_tests/txn_file_test.go`:
- Around line 259-268: Replace the non-deterministic time.Sleep with a
synchronization channel signaled by a new failpoint: instrument the commit path
with a "reached prewrite" failpoint that calls close(prewriteReachedCh) (or
sends on it) when the commit goroutine reaches the paused state, then in the
test wait on that channel before calling cluster.Split(...) instead of sleeping;
keep the existing goroutine that runs txn.Commit(ctx), ensure you still disable
the "tikvclient/invalidCacheAndRetry" failpoint after split, and preserve the
final <-done wait to ensure Commit finishes. Use identifiers txn.Commit,
cluster.Split, failpoint.Disable and the done channel to locate and update the
test and add the new failpoint signal in the commit execution path.
In `@txnkv/transaction/2pc.go`:
- Line 343: MutationsHasDataInRange is an exported function without a Go doc
comment; either add a clear Go doc comment describing its purpose, parameters
(mutations, start, end), return values (firstDataKey, found bool) and behavior,
or make it unexported by renaming it to mutationsHasDataInRange if it is not
part of the public API; update all call sites accordingly (look for
MutationsHasDataInRange references) so the package surface adheres to the
exported-identifiers documentation rule.
In `@txnkv/transaction/txn_file_test.go`:
- Around line 51-56: The test's genRndChunkIDs uses package-level math/rand
causing non-determinism; replace it with a local RNG created via
rand.New(rand.NewSource(<fixedSeed>)) (e.g., 42) and use that RNG (r.Intn) for n
and id generation inside genRndChunkIDs so the test data is deterministic and
not affected by global RNG state; ensure the seed is fixed and deterministic for
the test run and that all rand.* calls in genRndChunkIDs are switched to the
local RNG variable.
🪄 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: 4fc0fea7-71aa-49c6-b6f1-29a2e40dc67c
📒 Files selected for processing (12)
config/client.gointegration_tests/txn_file_test.gokv/variables.gometrics/metrics.gometrics/shortcuts.gotikv/split_region.gotxnkv/transaction/2pc.gotxnkv/transaction/2pc_test.gotxnkv/transaction/txn.gotxnkv/transaction/txn_file.gotxnkv/transaction/txn_file_test.gotxnkv/txnlock/lock_resolver.go
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
pingyu
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@util/misc.go`:
- Around line 179-198: Add Go doc comments for the exported util functions
GetMaxStartKey and GetMinEndKey. Describe their comparison semantics and make
the empty-key behavior explicit, including that an empty start key acts as an
unbounded low value and an empty end key acts as an unbounded high value. Keep
the comments attached to the function declarations so the public API in util is
properly documented.
🪄 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: afc08df7-a227-43a6-bbe6-2a9fc280e62f
📒 Files selected for processing (12)
integration_tests/txn_file_test.gointernal/locate/region_cache.gointernal/resourcecontrol/resource_control.gokv/variables.gometrics/metrics.gotikv/kv_test.gotxnkv/transaction/2pc.gotxnkv/transaction/test_probe.gotxnkv/transaction/txn.gotxnkv/transaction/txn_file.goutil/misc.goutil/misc_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- kv/variables.go
- metrics/metrics.go
- txnkv/transaction/txn.go
- txnkv/transaction/2pc.go
- integration_tests/txn_file_test.go
- txnkv/transaction/txn_file.go
| } | ||
|
|
||
| commitBo := retry.NewBackofferWithVars(ctx, int(CommitMaxBackoff), c.txn.vars) | ||
| c.commitTS, err = c.store.GetTimestampWithRetry(commitBo, c.txn.GetScope()) |
There was a problem hiding this comment.
Should the txn-file path use the same commitTS preparation/check path as normal 2PC here?
Normal 2PC gets the commitTS through txn.GetTimestampForCommit(...), then checks schema validity, MaxTxnTimeUse, and commitTSUpperBoundCheck before committing. This path calls store.GetTimestampWithRetry(...) directly, and the CommitTsExpired retry path does the same, so it seems to bypass SetCommitWaitUntilTSO, schema lease validation, and commitTS upper-bound constraints.
| } | ||
|
|
||
| prewriteBo := retry.NewBackofferWithVars(ctx, int(PrewriteMaxBackoff.Load()), c.txn.vars) | ||
| err = c.executeTxnFileAction(prewriteBo, c.txnFileCtx.slice, txnFilePrewriteAction{}) |
There was a problem hiding this comment.
Should txn-file either handle binlog the same way as normal 2PC, or fall back when c.shouldWriteBinlog() is true?
The normal 2PC path calls binlog.Prewrite(...) before KV prewrite and later calls binlog.Commit(...) or binlog.Skip(...). I don't see equivalent handling in executeTxnFile, and useTxnFile does not appear to exclude transactions with a binlog executor. This may cause a large txn-file transaction to commit to TiKV without producing the corresponding binlog.
There was a problem hiding this comment.
Fixed in d50071d.
As the txn file feature is support in TiDBCloud only, simply disable txn file when "should write binlog".
| c.setUndeterminedErr(errors.WithStack(sender.GetRPCError())) | ||
| } | ||
| // Unexpected error occurs, return it. | ||
| if err != nil { |
There was a problem hiding this comment.
Should this return ErrResultUndetermined for primary txn-file commit RPC errors, matching normal 2PC?
| ResourceGroupName: c.resourceGroupName, | ||
| }, | ||
| }) | ||
| markTxnFileRetryRequest(req, bo) |
There was a problem hiding this comment.
Is the txn-file path expected to preserve dynamic resource-group tags from resourceGroupTagger?
The normal prewrite/commit/rollback paths call c.resourceGroupTagger(req) when c.resourceGroupTag == nil && c.resourceGroupTagger != nil. I don't see the same handling for txn-file prewrite, commit, or rollback requests, so a transaction may lose its dynamic resource-group tag once it crosses the txn-file threshold.
There was a problem hiding this comment.
applyTxnFilePrewriteResourceGroupTagger skips the tagger entirely when sampleKeys is empty. This loses more than the key-derived table/row label: TiDB's ResourceGroupTagBuilder still encodes the keyspace, SQL digest, and plan digest when the key is empty. The current test explicitly expects the tag to be absent for such a batch, so this remains different from normal 2PC. Please invoke the tagger even when no representative key is available; only the key-derived fields need to be omitted.
| ConstLabels: constLabels, | ||
| }, []string{LblResult}) | ||
|
|
||
| TiKVTxnFileRequestCounter = prometheus.NewCounterVec( |
There was a problem hiding this comment.
Looks like the new txn-file metrics may be missing from RegisterMetrics()?
| // TxnChunkWriterConcurrency is the concurrency to request the txn chunk writer for file-based txn. | ||
| TxnChunkWriterConcurrency uint `toml:"txn-chunk-writer-concurrency" json:"txn-chunk-writer-concurrency"` | ||
| // TxnChunkMaxSize is the maximum size of a txn chunk of file-based txn. | ||
| TxnChunkMaxSize uint64 `toml:"txn-chunk-max-size" json:"txn-chunk-max-size"` |
There was a problem hiding this comment.
Maybe we should validate the new txn-file config values before accepting them, especially TxnChunkMaxSize > 0? If this is set to 0, the txn-file path can divide by zero when calculating chunk counts or parallelism, so rejecting or normalizing it in Valid() would make the failure mode clearer.
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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_file_test.go (1)
558-804: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a duplicate-ID case to
TestChunkSliceSortAndDedup
The test only covers duplicate IDs with identical range metadata, so it doesn’t pin down the first-occurrence-wins behavior when duplicates have differentsmallest/biggestvalues.🤖 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 `@txnkv/transaction/txn_file_test.go` around lines 558 - 804, Extend TestChunkSliceSortAndDedup with a deterministic duplicate-ID case where repeated IDs have different txnChunkRange metadata, and assert that sortAndDedup retains the range from the first occurrence while producing sorted, deduplicated chunkIDs. Keep the existing randomized coverage unchanged.
🧹 Nitpick comments (2)
txnkv/transaction/txn_file_test.go (2)
548-548: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant embedded-field selector.
Static analysis (staticcheck QF1008) flags this:
ttlManageris embedded intwoPhaseCommitter, socommitter.ttlManager.statecan be written ascommitter.state.♻️ Proposed simplification
- committer.ttlManager.state = stateRunning + committer.state = stateRunning🤖 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 `@txnkv/transaction/txn_file_test.go` at line 548, In the test setup around the embedded twoPhaseCommitter field, update the assignment to use the promoted state field directly via committer.state instead of the redundant committer.ttlManager.state selector.Source: Linters/SAST tools
491-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDirect manipulation of unexported package singletons (
once,cli,errCli,scheme) for test setup is fragile.Resetting these package-level vars works because tests in this file run sequentially, but it silently couples the test to the internal implementation names/shape of the chunk-writer client singleton in
txn_file.go. Any rename/refactor of that singleton (or a futuret.Parallel()addition elsewhere in the package) breaks or races this test without an obvious signal. Consider exposing a small internal test hook (e.g., aresetChunkWriterClientForTest()intxn_file.go) instead of poking at the vars directly.🤖 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 `@txnkv/transaction/txn_file_test.go` around lines 491 - 506, The test directly resets the chunk-writer singleton internals, coupling it to implementation details. Add an internal resetChunkWriterClientForTest hook near the singleton implementation in txn_file.go that clears once, cli, errCli, and scheme, then replace both direct reset blocks in the test with calls to that hook while preserving the existing configuration restoration.
🤖 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 `@txnkv/transaction/txn_file_test.go`:
- Around line 484-488: Update the HTTP handler in the chunkWriter test server to
replace require.Equal and require.NoError with non-terminating assertions such
as t.Errorf or assert calls. Keep the method validation and response-writing
behavior unchanged, ensuring failures from the request goroutine are reported
without invoking FailNow.
---
Outside diff comments:
In `@txnkv/transaction/txn_file_test.go`:
- Around line 558-804: Extend TestChunkSliceSortAndDedup with a deterministic
duplicate-ID case where repeated IDs have different txnChunkRange metadata, and
assert that sortAndDedup retains the range from the first occurrence while
producing sorted, deduplicated chunkIDs. Keep the existing randomized coverage
unchanged.
---
Nitpick comments:
In `@txnkv/transaction/txn_file_test.go`:
- Line 548: In the test setup around the embedded twoPhaseCommitter field,
update the assignment to use the promoted state field directly via
committer.state instead of the redundant committer.ttlManager.state selector.
- Around line 491-506: The test directly resets the chunk-writer singleton
internals, coupling it to implementation details. Add an internal
resetChunkWriterClientForTest hook near the singleton implementation in
txn_file.go that clears once, cli, errCli, and scheme, then replace both direct
reset blocks in the test with calls to that hook while preserving the existing
configuration restoration.
🪄 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: 519fb185-007a-4c49-9860-68a2d948d972
📒 Files selected for processing (2)
txnkv/transaction/txn_file.gotxnkv/transaction/txn_file_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- txnkv/transaction/txn_file.go
Signed-off-by: Ping Yu <yuping@pingcap.com>
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
| var _ txnFileAction = (*txnFilePrewriteAction)(nil) | ||
|
|
||
| func (a txnFilePrewriteAction) executeBatch(c *twoPhaseCommitter, bo *retry.Backoffer, batch chunkBatch) (*tikvrpc.Response, error) { | ||
| primaryLock := c.txnFileCtx.slice.chunkRanges[0].smallest |
There was a problem hiding this comment.
P2: Should not use the minimum key of the first chunk as the transaction primary
PrimaryLock cannot always be derived from the first chunk. initKeysAndMutations skips CheckNotExists/SharedLock when choosing c.primaryKey, so a smaller prewrite-only mutation can precede the real primary. In that case this either fails with primary out of first batch, or makes the write locks point to a key that has no lock. Please use c.primary() and explicitly select the batch containing it; the pos == 0 assumption in MutationsHasDataInRange needs the same treatment.
There was a problem hiding this comment.
Fixed by b30dcc8.
The change also remove the "Always return primary key if it's in the range". I check server side codes, and find that the primary is not necessary. The keys are used to check transaction committed/rollback on commit/rollback. So `isOpForWrite" should be enough.
| minMutationSize = minMutationSize / 2 | ||
| } | ||
|
|
||
| if c.txn.isPessimistic || |
There was a problem hiding this comment.
P1: Should exclude pipelined transactions from the txn-file path
Commit intentionally skips initKeysAndMutations for a pipelined transaction, so committer.mutations remains nil. If this gate succeeds, buildTxnFiles dereferences it at mutations.GetKey(0) and panics. More importantly, mutations already flushed by pipelined DML are no longer available to serialize into txn files. Please exclude c.txn.isPipelined here unless the txn-file path is extended to handle flushed mutations explicitly.
| } | ||
|
|
||
| // Extract lock from key error | ||
| lock, err1 := txnlock.ExtractLockFromKeyErr(keyErr) |
There was a problem hiding this comment.
P1: Should expand shared-lock wrappers before resolving locks
ExtractLockFromKeyErr returns the shared-lock wrapper when SharedLockInfos is present, but ResolveLocksWithOpts explicitly rejects Lock.IsShared() and requires callers to resolve the inner locks one by one. As a result, txn-file prewrite fails whenever it encounters a shared lock. Please mirror or factor the normal prewrite extraction logic here; the same issue exists in handleSplitRegionKeyErrors, and this path should also preserve NoResolvePolicy.
There was a problem hiding this comment.
One more detail when mirroring the normal prewrite path: the newer-TS/write-conflict check needs to use each inner lock’s TxnID, and RecordResolvingLocks, UpdateResolvingLocks, and ResolveLocksWithOpts should all receive the expanded inner locks rather than the wrapper.
Could we also add a multi-holder test with one expired holder and one active holder, verifying that cleanup removes only the expired holder while the active holder remains and keeps the prewrite waiting? Reusing the normal prewrite resolver options, including PessimisticRegionResolve, would also keep the cleanup behavior and RPC profile aligned.
| c.mu.RUnlock() | ||
| if !committed && !undetermined { | ||
| if c.txnFileCtx.slice.Len() > 0 { | ||
| err1 := c.executeTxnFileAction(retry.NewBackofferWithVars(ctx, int(CommitMaxBackoff), c.txn.vars), c.txnFileCtx.slice, txnFileRollbackAction{}) |
There was a problem hiding this comment.
P2: Should not reuse the commit context for rollback cleanup
If prewrite succeeds and the caller then cancels or times out the commit, this context is already canceled, so the rollback RPC cannot be sent and the locks remain until TTL cleanup. Normal 2PC derives cleanup from c.store.Ctx() for this reason. Please use an independent store-scoped cleanup context here, preferably through the store pool.
| if config.TxnChunkMaxSize == 0 { | ||
| return fmt.Errorf("txn-chunk-max-size should be greater than 0") | ||
| } | ||
| if config.TxnChunkMaxSize > math.MaxInt { |
There was a problem hiding this comment.
P2: Should reject chunk sizes that make the parallelism limit zero
This validation still accepts TxnChunkMaxSize values greater than MaxTxnChunkSizeInParallel (4 GiB). For such a valid value, 4 GiB / TxnChunkMaxSize becomes 0 and NewRateLimit(0) deadlocks the multi-secondary path. Please reject values above the parallel budget and defensively clamp the calculated rate limit to at least 1.
|
|
||
| buildBo := retry.NewBackofferWithVars(ctx, int(BuildTxnFileMaxBackoff.Load()), c.txn.vars) | ||
|
|
||
| rcInterceptor := client.ResourceControlInterceptor.Load() |
There was a problem hiding this comment.
P2: Should honor the normal resource-control enable and bypass checks
Loading ResourceControlInterceptor directly bypasses ResourceControlSwitch and the normal empty-group, background-request, and RequestInfo.Bypass checks. DisableResourceControl leaves the interceptor pointer installed, so txn-file transactions can still be charged—or fail while looking up an empty resource group—when normal RPCs would skip resource control. Please reuse or factor the same selection logic used by getResourceControlInfo before applying the aggregate txn-file charge.
| if config.GetGrpcKeepAliveTimeout() < time.Millisecond*50 { | ||
| return fmt.Errorf("grpc-keepalive-timeout should be at least 0.05, but got %f", config.GrpcKeepAliveTimeout) | ||
| } | ||
| return validateTxnFileConfig(config) |
There was a problem hiding this comment.
P2: Could txn-file validation be skipped while the feature is disabled?
TiKVClient.Valid now validates TxnChunkMaxSize and TxnChunkWriterConcurrency even when TxnChunkWriterAddr is empty, which currently means txn-file is disabled. Is this intended?
An existing caller that constructs TiKVClient directly, or decodes an old configuration into a zero-valued struct, can now fail validation without using txn-file at all. Callers starting from DefaultTiKVClient are unaffected because these fields already have defaults, but client-go consumers are not necessarily required to do that.
Would it make sense to validate these txn-file-only fields only when TxnChunkWriterAddr is configured, or normalize the zero values before validation? A feature-off compatibility test using an old-style configuration would also help preserve this behavior.
|
|
||
| spResp := resp.Resp.(*kvrpcpb.SplitRegionResponse) | ||
|
|
||
| keyErrs := spResp.GetErrors() |
There was a problem hiding this comment.
P1: Is the lock-aware SplitRegions behavior intended for all callers?
This handling is added to the generic KVStore.SplitRegions path, so it also affects existing external callers and normal 2PC's best-effort pre-split, even when txn-file is not used.
Previously, a split request that encountered a locked key returned promptly and normal 2PC continued with prewrite. With this change, the same request can resolve locks, sleep for an active lock's TTL, and retry within the split backoff budget. It can also return an error for a shared-lock wrapper because ResolveLocksWithOpts does not accept shared locks directly.
Was this intended as a general behavior change for SplitRegions, or is it only needed by txn-file pre-split? If it is txn-file-specific, could we keep the lock-aware retry in that path? If it is intended as a generic fix, separating it from the feature and adding compatibility coverage for active, expired, and shared locks may make the behavior change easier to evaluate.
| minMutationSize = minMutationSize / 2 | ||
| } | ||
|
|
||
| if c.txn.isPessimistic || |
There was a problem hiding this comment.
Could we also exclude c.hasSharedLocks here? isPessimistic covers the normal shared-lock path today, but hasSharedLocks is the post-init capability signal and is already used to exclude async commit and 1PC. Since txn-file prewrite currently does not support Op_SharedLock, relying on the transaction mode alone makes this boundary implicit. An eligibility test with hasSharedLocks set would also document that these transactions always fall back to normal 2PC.
| PrimaryLock: primaryLock, | ||
| LockTtl: c.lockTTL, | ||
| MaxCommitTs: c.maxCommitTS, | ||
| AssertionLevel: kvrpcpb.AssertionLevel_Off, |
There was a problem hiding this comment.
P1: Preserve strict mutation assertions in the txn-file path, or fall back to normal 2PC
Normal prewrite copies IsAssertExists / IsAssertNotExist into each Mutation.Assertion and sends c.txn.assertionLevel. The txn-file path instead serializes only key/op/value into the chunks and unconditionally sends AssertionLevel_Off; useTxnFile also does not exclude transactions that have strict assertions. The server cannot reconstruct these assertions from the chunk format.
This is observable for optimistic TiDB DML: update and delete mutations carry AssertExist, and NextGen defaults tidb_txn_assertion_level to STRICT. Once such a transaction crosses the txn-file size threshold, an invariant violation that normal 2PC would return as ErrAssertionFailed can be prewritten and committed without that check, provided there is no other MVCC error. Op_Insert does not cover this case because update/delete remain Put/Del with an independent assertion.
Please either extend the txn-chunk protocol and server implementation to preserve AssertExist / AssertNotExist together with the request assertion level, or conservatively fall back to normal 2PC when c.txn.assertionLevel != AssertionLevel_Off and any mutation has an Exist/NotExist assertion. Please also add a real txn-file integration test that intentionally violates AssertExist or AssertNotExist and expects ErrAssertionFailed.
There was a problem hiding this comment.
The fallback in f0a60e9 looks good. Could we also add the integration coverage mentioned above? An integration test with txn-file enabled that violates AssertExist or AssertNotExist and verifies Commit returns ErrAssertionFailed would help confirm the fallback works end to end. Ideally, it could also check that no txn chunk is written.
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
[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 |
| return | ||
| } | ||
|
|
||
| err = c.afterExecuteTxnFile(rcInterceptor, reqInfo, ruDetails) |
There was a problem hiding this comment.
P2: Do not return a fallible custom resource-accounting error after the primary has committed
When resource control is enabled for a non-empty, non-background, non-bypassed resource group, executeTxnFileAction has already received a definitive primary-commit success and set c.mu.committed before this call. A custom interceptor installed through tikv.SetResourceControlInterceptor is allowed to return an error from OnResponse; propagating that error reports the transaction as failed even though its data is committed, which can make the caller retry an already committed transaction. The current built-in PD controller returns nil from OnResponse, so the default implementation is not affected today, but the public custom-interceptor path can still produce this contradictory outcome. Please preserve the successful commit result—for example, log the post-commit accounting failure instead of returning it—and add a test where OnResponse returns an error after a successful primary commit.
| if err1 != nil { | ||
| return nil, err1 | ||
| } | ||
| return resp, nil |
There was a problem hiding this comment.
P2: Propagate key errors from the primary rollback
This returns success for every non-transport response, while the primary path never calls extractKeyError; only the secondary/common batch path does. Consequently, a BatchRollbackResponse.Error from the primary region is silently treated as successful cleanup. This masks a failed primary cleanup and, depending on the returned key error, transaction locks may remain until TTL-based cleanup; some errors, such as an already-committed transaction, do not imply a remaining lock. Please validate the rollback response here—or handle extractKeyError in the common primary path—and add primary-rollback key-error coverage.
| ) | ||
|
|
||
| scheme = "http://" | ||
| transport := &http.Transport{ |
There was a problem hiding this comment.
P2: Release the txn-file uploader idle HTTP connections
This process-global custom transport has IdleConnTimeout == 0, so it imposes no client-side idle expiry, and it is never passed to CloseIdleConnections. Consequently, txn-file sockets can survive KVStore.Close until the peer or process closes them. The retention is bounded by MaxIdleConnsPerHost (20 for a stable single writer address) and MaxIdleConns (100 globally across hosts), but it still violates the client lifecycle and can retain stale descriptors across stores or writer-address changes. #2038 already adds the required close hook; please either pull that lifecycle fix into this PR or make the merge/release dependency explicit. A bounded IdleConnTimeout would also provide a useful fallback.
| zap.Stringer("action", action)) | ||
| return nil | ||
| } | ||
| errGo := c.txn.spawnWithStorePool(func() { |
There was a problem hiding this comment.
P2: Release MemDB values before committing
Unlike regular 2PC, the txn-file path does not call DiscardValues after the values have been uploaded. The asynchronous secondary closure retains the committer, and therefore the entire MemDB, until it finishes. Since txn-file transactions are large and secondary retries may take minutes, concurrent commits can significantly increase memory usage or cause OOM.
Please discard the values before committing the primary, once they are no longer needed. The keys and metadata required by secondary retries will remain available.
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
2 participants
Changes
Cherry pick "file based transaction" PRs to master:
Integration test in next PR: txnkv: add txn-file docker integration tests and close idle HTTP connections #2038
Summary by CodeRabbit