txnkv: add txn-file docker integration tests and close idle HTTP connections - #2038
txnkv: add txn-file docker integration tests and close idle HTTP connections#2038pingyu wants to merge 42 commits into
Conversation
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>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
ti-chi-bot
Bot
added
dco-signoff: yes
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds configurable transaction-file commits with chunk upload, region-aware 2PC, retries, metrics, client cleanup, native integration tests, and a Docker Compose test environment. ChangesTransaction-file commit support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant KVTxn
participant ChunkWriter
participant TiKV
Client->>KVTxn: commit transaction
KVTxn->>ChunkWriter: upload transaction chunks
KVTxn->>TiKV: prewrite and commit region batches
TiKV-->>KVTxn: commit responses or retry errors
KVTxn-->>Client: commit result
Possibly related PRs
Suggested labels: 🚥 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 |
|
Manual Test |
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
metrics/metrics.go (1)
1091-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the seconds suffix to the file duration metric.
TiKVTxnFileDurationrecords seconds (dur.Seconds()), but exportstxn_file_duration. Rename the exported name totxn_file_duration_secondsto match existing second-based duration metrics.🤖 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 1091 - 1099, Update the Name field in TiKVTxnFileDuration to use the exported metric name txn_file_duration_seconds, preserving the existing histogram configuration and labels.integration_tests_in_docker/txn_file/helpers_test.go (3)
86-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe txn-file chunk size is declared twice. One value configures the client and a separate literal drives the chunk-count assertion. Nothing keeps the two equal, so a change to one silently weakens the test at
integration_tests_in_docker/txn_file/txn_file_test.goline 58.
integration_tests_in_docker/txn_file/helpers_test.go#L86-L94: addtxnChunkMaxSize = 256to theconstblock at lines 38-43 and setconf.TiKVClient.TxnChunkMaxSize = txnChunkMaxSize.integration_tests_in_docker/txn_file/txn_file_test.go#L30-L33: remove the localtxnChunkMaxSizedeclaration and use the shared constant.🤖 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_in_docker/txn_file/helpers_test.go` around lines 86 - 94, The txn-file chunk size is duplicated between configuration and assertions; define the shared txnChunkMaxSize constant in integration_tests_in_docker/txn_file/helpers_test.go lines 38-43, use it in the config update around lines 86-94, and remove the local declaration in integration_tests_in_docker/txn_file/txn_file_test.go lines 30-33 so the assertion uses the shared constant.
229-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the backoff budget from the context timeout.
regionLocateBackoffis a backoff budget passed totikv.NewBackofferWithVars. Line 229 reuses the same constant as a millisecond duration for the context deadline. The two values have different units. If a maintainer tunes the backoff budget, the request timeout changes at the same time without intent.♻️ Proposed fix
regionLocateBackoff = 1000 + regionLocateTimeout = time.Second directGetAttempts = 5 )- ctx, cancel := context.WithTimeout(context.Background(), time.Duration(regionLocateBackoff)*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), regionLocateTimeout)🤖 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_in_docker/txn_file/helpers_test.go` around lines 229 - 231, Separate the context timeout from the backoff budget in the setup surrounding NewBackofferWithVars: replace the use of regionLocateBackoff for context.WithTimeout with a dedicated timeout duration, while continuing to pass regionLocateBackoff as the backoffer budget.
159-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInvalidate the cached region for every split key.
Line 162 invalidates only the region that contained
splitKeys[0]. If the split keys span more than one pre-existing region, the region cache keeps stale entries for the other ranges.locateKeyOncethen returns the old region ID for every key in those ranges,regionGroupsMatchnever becomes true, andrequire.Eventuallyfails only after the 30s timeout. The current tests pass because all split keys fall in one region, so this is a latent fragility rather than a present failure.♻️ Proposed fix
- old := locateKey(t, store, splitKeys[0]) + stale := make([]*tikv.KeyLocation, 0, len(splitKeys)) + for _, splitKey := range splitKeys { + stale = append(stale, locateKey(t, store, splitKey)) + } _, err := store.SplitRegions(context.Background(), splitKeys, false, nil) require.NoError(t, err) - store.GetRegionCache().InvalidateCachedRegion(old.Region) + for _, location := range stale { + store.GetRegionCache().InvalidateCachedRegion(location.Region) + }🤖 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_in_docker/txn_file/helpers_test.go` around lines 159 - 167, Update the split-region test setup around SplitRegions to locate and invalidate the cached region for every key in splitKeys, rather than only splitKeys[0]. Preserve the existing error assertion and eventual region-group verification, ensuring each pre-existing region represented by the split keys has its cache entry invalidated.integration_tests_in_docker/docker-compose/bootstrap/create-keyspace.sh (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
${KEYSPACE_NAME}inis_valid_local_keyspace.The function hardcodes
local_normalalthough line 5 definesKEYSPACE_NAME. IfKEYSPACE_NAMEchanges, the validation checks the wrong name and the loop at lines 78-91 always fails. The error message at line 88 also hardcodes the name.♻️ Proposed fix
is_valid_local_keyspace() { - has_json_field "${local_output}" name local_normal && + has_json_field "${local_output}" name "${KEYSPACE_NAME}" && has_json_field "${local_output}" state ENABLED && has_json_field "${local_output}" gc_management_type keyspace_level }Also update the message at line 88:
- echo "local_normal keyspace validation failed" >&2 + echo "${KEYSPACE_NAME} keyspace validation failed" >&2🤖 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_in_docker/docker-compose/bootstrap/create-keyspace.sh` around lines 36 - 40, Update is_valid_local_keyspace to validate the keyspace name using the KEYSPACE_NAME variable instead of the hardcoded local_normal value. Also update the related error message in the loop to interpolate KEYSPACE_NAME so validation and diagnostics remain consistent when the configured name changes.integration_tests_in_docker/txn_file/txn_file_test.go (1)
146-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an observable signal instead of reimplementing txn file chunking.
txnChunkEntrySizeandtxnFileChunkCountduplicate the encoder arithmetic intxnkv/transaction/txn_file.go, including key length, op, value length, and CRC splitting. When the encoder layout or packing rule changes, this test can stop covering multiple commits without failure. Assert the chunk count through the uploaded chunk count ortxnFileRequestChunkCount()instead.🤖 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_in_docker/txn_file/txn_file_test.go` around lines 146 - 165, Replace the duplicated txnChunkEntrySize and txnFileChunkCount arithmetic with an observable chunk-count assertion using the uploaded chunk count or the existing txnFileRequestChunkCount() helper. Update callers in the transaction file test to derive expected chunks through the encoder/request path, so changes to txn file layout or packing rules remain covered without maintaining parallel sizing logic.integration_tests/txn_file_test.go (1)
62-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated test-environment setup into a shared helper.
The
httptest.NewServerchunk-writer stub (lines 63-73 and 172-182), the global config override (lines 76-82 and 185-191), and the mock TiKV/PD bootstrap (lines 84-89 and 193-198) are copy-pasted betweenTestTxnFilePrewriteTxnSizeandTestTxnFilePrewriteTxnSizeAfterRegionRegroup. Extract a helper, for examplesetupTxnFileTestEnv(t *testing.T) (*httptest.Server, *mocktikv-based cluster/store, func()), that returns the server, cluster, store, and a single cleanup closure.♻️ Proposed helper extraction
func newTxnFileTestServer(t *testing.T) (*httptest.Server, *atomic.Uint64) { var chunkIDCounter atomic.Uint64 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } id := chunkIDCounter.Add(1) resp, _ := json.Marshal(map[string]uint64{"chunk_id": id}) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) _, _ = w.Write(resp) })) return srv, &chunkIDCounter } func withTxnFileConfig(t *testing.T, srv *httptest.Server, maxChunkSize uint64) func() { origCfg := config.GetGlobalConfig() newCfg := *origCfg newCfg.TiKVClient.TxnChunkWriterAddr = srv.Listener.Addr().String() newCfg.TiKVClient.TxnChunkMaxSize = maxChunkSize newCfg.TiKVClient.TxnFileMinMutationSize = 1 config.StoreGlobalConfig(&newCfg) return func() { config.StoreGlobalConfig(origCfg) } }Also applies to: 171-198
🤖 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 62 - 89, Extract the duplicated setup used by TestTxnFilePrewriteTxnSize and TestTxnFilePrewriteTxnSizeAfterRegionRegroup into shared helpers, including the chunk-writer stub, global configuration override, and mock TiKV/PD bootstrap. Anchor the refactor around newTxnFileTestServer, withTxnFileConfig, and a setup helper that returns the server, cluster/store resources, and one cleanup closure; update both tests to use it while preserving existing configuration and resource cleanup behavior.txnkv/transaction/2pc.go (1)
343-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the sorted-mutations precondition.
sort.Searchrequiresmutationsto be sorted by key in ascending order. The doc comment does not state this precondition. Callers outside this package can pass unsortedCommitterMutationsand get silently wrong results.📝 Proposed doc update
// MutationsHasDataInRange returns whether mutations has data in the range [start, end). // If it has, it returns the primary or first write key in the range. // Note that the firstDataKey can be empty when the range contains only non-write ops (and not the primary at pos 0). +// The mutations must be sorted by key in ascending order. func MutationsHasDataInRange(mutations CommitterMutations, start []byte, end []byte) ([]byte /* firstDataKey */, bool) {🤖 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` around lines 343 - 358, Update the doc comment for MutationsHasDataInRange to explicitly state that mutations must be sorted by key in ascending order before calling this function. Keep the existing range and return-value documentation unchanged.txnkv/transaction/txn_file.go (2)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment for
PreSplitRegionChunks.
PreSplitRegionChunksis exported and has no doc comment.MaxTxnChunkSizeInParallelnext to it has one.📝 Proposed doc update
const ( + // PreSplitRegionChunks is the number of txn-file chunks per region that + // triggers a pre-split before prewrite. PreSplitRegionChunks = 4As per coding guidelines: "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/txn_file.go` around lines 66 - 71, Add a clear Go doc comment immediately above the exported PreSplitRegionChunks constant, describing its purpose and matching the existing documentation style used for MaxTxnChunkSizeInParallel.Source: Coding guidelines
1298-1328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe HTTP client freezes configuration at first use.
sync.Oncebuilds the client one time per process.Timeoutderives fromBuildTxnFileMaxBackoffand the transport derives fromcfg.Security. Later changes to either value have no effect, becauseonce.Donever runs again. The test code intxnkv/transaction/txn_file_test.goworks around this by resettingonce,cli,errCli, andschemedirectly.If the security configuration is expected to be reloadable at runtime, rebuild the client on change. If not, state the one-time initialization in a comment on the var block.
🤖 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.go` around lines 1298 - 1328, Clarify the intended lifecycle of getHTTPClient and its configuration: if cfg.Security or BuildTxnFileMaxBackoff must support runtime changes, replace the once.Do initialization with change-aware client rebuilding while preserving TLS/error handling; otherwise add a comment at the once/cli/errCli/scheme variable block documenting that the HTTP client intentionally initializes only once and configuration changes require process restart.txnkv/transaction/txn_file_test.go (2)
1004-1005: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion does not check what the comment claims.
var _ apicodec.KeyspaceID = apicodec.NullspaceIDonly asserts thatNullspaceIDhas typeapicodec.KeyspaceID. It does not verify that the testRegionCachecodec returns keyspace ID 0. Either assert the value inside a test, or delete the declaration and the comment.♻️ Proposed change
-// Ensure the codec used by the test RegionCache returns keyspace ID 0 (codecV1). -var _ apicodec.KeyspaceID = apicodec.NullspaceIDAdd the real check to
TestBuildTxnFilesEntryCountinginstead:require.Equal(apicodec.NullspaceID, regionCache.Codec().GetKeyspaceID())🤖 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 1004 - 1005, Remove the misleading compile-time declaration and update TestBuildTxnFilesEntryCounting to assert that regionCache.Codec().GetKeyspaceID() equals apicodec.NullspaceID, preserving the comment only if it accurately describes this runtime check.
942-949: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead statements in the seed loop.
flagsis created and immediately discarded.opis discarded too. Neither affectsmemDB. These lines are debug artifacts.♻️ Proposed cleanup
- for i, op := range ops { + for i := range ops { key := []byte(fmt.Sprintf("k%02d", i)) val := []byte(fmt.Sprintf("v%02d", i)) - flags := tikv.KeyFlags(0) - _ = flags - _ = op require.NoError(memDB.Set(key, val)) }Check whether
tikvis still used elsewhere in the file after this change.🤖 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 942 - 949, Remove the unused flags declaration and both discard statements from the seed loop in the test, while retaining key/value generation and memDB.Set. Afterward, remove the tikv import if no other references remain in txn_file_test.go.
🤖 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 `@integration_tests_in_docker/txn_file/main_test.go`:
- Around line 26-34: Update TestMain’s client logging setup so txn-file test
logs remain available after the container is removed: write txn-file.log to a
bind-mounted host path or tee the logger output to stderr while preserving file
logging. Ensure failures still expose client-side logs through the existing
docker diagnostics workflow.
In `@integration_tests/txn_file_test.go`:
- Around line 260-269: Replace the fixed sleep in the goroutine coordination
around txn.Commit with deterministic synchronization that confirms the commit
has reached the paused tikvclient/invalidCacheAndRetry failpoint before calling
cluster.Split and disabling the failpoint. Update the final wait on done to use
a bounded timeout and fail the test with a clear assertion if the commit does
not complete in time.
In `@tikv/split_region.go`:
- Around line 183-196: Ensure every key-error retry in the batch split flow
records backoff before recursively calling splitBatchRegionsReq. Update the
key-error branch in batchSendSingleRegion or make handleSplitRegionKeyErrors
perform an unconditional backoff, including when resolveLockRes.TTL is zero,
while preserving existing error handling.
In `@txnkv/transaction/txn_file.go`:
- Around line 971-978: Add an empty-slice guard in
twoPhaseCommitter.executeTxnFileAction after groupToBatches succeeds and before
accessing batches[0]. When no batches are returned, skip the primary-batch
execution path safely and continue or return according to the surrounding
transaction flow, while preserving existing handling for non-empty batches.
- Around line 1079-1083: Validate key and value lengths before serializing the
entry, rejecting lengths above the representable uint16 and uint32 limits
instead of narrowing them. Update the surrounding serialization method to return
descriptive errors for oversized keys or values, add the math import for the
bounds, and only write the length prefixes after validation succeeds.
---
Nitpick comments:
In `@integration_tests_in_docker/docker-compose/bootstrap/create-keyspace.sh`:
- Around line 36-40: Update is_valid_local_keyspace to validate the keyspace
name using the KEYSPACE_NAME variable instead of the hardcoded local_normal
value. Also update the related error message in the loop to interpolate
KEYSPACE_NAME so validation and diagnostics remain consistent when the
configured name changes.
In `@integration_tests_in_docker/txn_file/helpers_test.go`:
- Around line 86-94: The txn-file chunk size is duplicated between configuration
and assertions; define the shared txnChunkMaxSize constant in
integration_tests_in_docker/txn_file/helpers_test.go lines 38-43, use it in the
config update around lines 86-94, and remove the local declaration in
integration_tests_in_docker/txn_file/txn_file_test.go lines 30-33 so the
assertion uses the shared constant.
- Around line 229-231: Separate the context timeout from the backoff budget in
the setup surrounding NewBackofferWithVars: replace the use of
regionLocateBackoff for context.WithTimeout with a dedicated timeout duration,
while continuing to pass regionLocateBackoff as the backoffer budget.
- Around line 159-167: Update the split-region test setup around SplitRegions to
locate and invalidate the cached region for every key in splitKeys, rather than
only splitKeys[0]. Preserve the existing error assertion and eventual
region-group verification, ensuring each pre-existing region represented by the
split keys has its cache entry invalidated.
In `@integration_tests_in_docker/txn_file/txn_file_test.go`:
- Around line 146-165: Replace the duplicated txnChunkEntrySize and
txnFileChunkCount arithmetic with an observable chunk-count assertion using the
uploaded chunk count or the existing txnFileRequestChunkCount() helper. Update
callers in the transaction file test to derive expected chunks through the
encoder/request path, so changes to txn file layout or packing rules remain
covered without maintaining parallel sizing logic.
In `@integration_tests/txn_file_test.go`:
- Around line 62-89: Extract the duplicated setup used by
TestTxnFilePrewriteTxnSize and TestTxnFilePrewriteTxnSizeAfterRegionRegroup into
shared helpers, including the chunk-writer stub, global configuration override,
and mock TiKV/PD bootstrap. Anchor the refactor around newTxnFileTestServer,
withTxnFileConfig, and a setup helper that returns the server, cluster/store
resources, and one cleanup closure; update both tests to use it while preserving
existing configuration and resource cleanup behavior.
In `@metrics/metrics.go`:
- Around line 1091-1099: Update the Name field in TiKVTxnFileDuration to use the
exported metric name txn_file_duration_seconds, preserving the existing
histogram configuration and labels.
In `@txnkv/transaction/2pc.go`:
- Around line 343-358: Update the doc comment for MutationsHasDataInRange to
explicitly state that mutations must be sorted by key in ascending order before
calling this function. Keep the existing range and return-value documentation
unchanged.
In `@txnkv/transaction/txn_file_test.go`:
- Around line 1004-1005: Remove the misleading compile-time declaration and
update TestBuildTxnFilesEntryCounting to assert that
regionCache.Codec().GetKeyspaceID() equals apicodec.NullspaceID, preserving the
comment only if it accurately describes this runtime check.
- Around line 942-949: Remove the unused flags declaration and both discard
statements from the seed loop in the test, while retaining key/value generation
and memDB.Set. Afterward, remove the tikv import if no other references remain
in txn_file_test.go.
In `@txnkv/transaction/txn_file.go`:
- Around line 66-71: Add a clear Go doc comment immediately above the exported
PreSplitRegionChunks constant, describing its purpose and matching the existing
documentation style used for MaxTxnChunkSizeInParallel.
- Around line 1298-1328: Clarify the intended lifecycle of getHTTPClient and its
configuration: if cfg.Security or BuildTxnFileMaxBackoff must support runtime
changes, replace the once.Do initialization with change-aware client rebuilding
while preserving TLS/error handling; otherwise add a comment at the
once/cli/errCli/scheme variable block documenting that the HTTP client
intentionally initializes only once and configuration changes require process
restart.
🪄 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 Plus
Run ID: 84abe96f-5ce6-402d-a56c-4e18da2ccea2
⛔ Files ignored due to path filters (1)
integration_tests_in_docker/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
config/client.goconfig/config_test.gointegration_tests/txn_file_test.gointegration_tests_in_docker/README.mdintegration_tests_in_docker/docker-compose/Dockerfile.testintegration_tests_in_docker/docker-compose/Dockerfile.test.dockerignoreintegration_tests_in_docker/docker-compose/bootstrap/create-keyspace.shintegration_tests_in_docker/docker-compose/bootstrap/init-minio.shintegration_tests_in_docker/docker-compose/configs/pd.tomlintegration_tests_in_docker/docker-compose/configs/tikv-1.tomlintegration_tests_in_docker/docker-compose/configs/tikv-2.tomlintegration_tests_in_docker/docker-compose/configs/tikv-3.tomlintegration_tests_in_docker/docker-compose/configs/tikv-worker.tomlintegration_tests_in_docker/docker-compose/docker-compose.ymlintegration_tests_in_docker/docker-compose/run.shintegration_tests_in_docker/go.modintegration_tests_in_docker/txn_file/helpers_test.gointegration_tests_in_docker/txn_file/main_test.gointegration_tests_in_docker/txn_file/txn_file_test.gointernal/locate/region_cache.gointernal/resourcecontrol/resource_control.gokv/variables.gometrics/metrics.gometrics/shortcuts.gotikv/kv_test.gotikv/split_region.gotxnkv/client.gotxnkv/client_test.gotxnkv/transaction/2pc.gotxnkv/transaction/2pc_test.gotxnkv/transaction/test_probe.gotxnkv/transaction/txn.gotxnkv/transaction/txn_file.gotxnkv/transaction/txn_file_test.goutil/misc.goutil/misc_test.go
| const txnFileLogPath = "txn-file.log" | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| logFile, err := os.Create(txnFileLogPath) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "create txn-file test log: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| logger, props, err := log.InitLoggerWithWriteSyncer(&log.Config{Level: "info"}, logFile, logFile) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The client-side log is discarded on failure.
TestMain redirects all client logs to txn-file.log inside the test container and away from stdout. integration_tests_in_docker/docker-compose/run.sh line 179 runs the test service with compose run --rm, so the container is removed when the run ends. The diagnostics function at run.sh lines 128-137 collects logs from pd, tikv-1, tikv-2, tikv-3, and tikv-worker only. When a txn-file test fails, no client-side log remains for analysis.
Write the log to a bind-mounted path, or tee it to stderr, so the log survives the container removal.
🤖 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_in_docker/txn_file/main_test.go` around lines 26 - 34,
Update TestMain’s client logging setup so txn-file test logs remain available
after the container is removed: write txn-file.log to a bind-mounted host path
or tee the logger output to stderr while preserving file logging. Ensure
failures still expose client-side logs through the existing docker diagnostics
workflow.
| done := make(chan struct{}) | ||
| go func() { | ||
| _ = txn.Commit(ctx) | ||
| close(done) | ||
| }() | ||
|
|
||
| time.Sleep(3 * time.Second) | ||
| cluster.Split(regionID, cluster.AllocID(), []byte("h"), []uint64{peerID}, peerID) | ||
| require.Nil(failpoint.Disable("tikvclient/invalidCacheAndRetry")) | ||
| <-done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the fixed sleep with deterministic synchronization and add a timeout on <-done.
Line 266 sleeps 3 seconds to let the committing goroutine reach the paused tikvclient/invalidCacheAndRetry failpoint before the main goroutine splits the region and disables the failpoint. Under CI load, 3 seconds may not be enough, or may be far more than needed, making the test either flaky or slower than necessary.
Line 269 then waits on <-done with no timeout. If the retry/regroup path stalls for any reason, the test hangs instead of failing with a clear message.
🕒 Proposed fix: bound the wait with a timeout
- time.Sleep(3 * time.Second)
+ require.Eventually(t, func() bool {
+ mu.Lock()
+ defer mu.Unlock()
+ return len(captured) >= 1
+ }, 5*time.Second, 10*time.Millisecond, "expected the first prewrite to be captured before splitting")
cluster.Split(regionID, cluster.AllocID(), []byte("h"), []uint64{peerID}, peerID)
require.Nil(failpoint.Disable("tikvclient/invalidCacheAndRetry"))
- <-done
+ select {
+ case <-done:
+ case <-time.After(30 * time.Second):
+ t.Fatal("timed out waiting for txn.Commit to finish")
+ }🤖 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 260 - 269, Replace the fixed
sleep in the goroutine coordination around txn.Commit with deterministic
synchronization that confirms the commit has reached the paused
tikvclient/invalidCacheAndRetry failpoint before calling cluster.Split and
disabling the failpoint. Update the final wait on done to use a bounded timeout
and fail the test with a clear assertion if the commit does not complete in
time.
|
|
||
| keyErrs := spResp.GetErrors() | ||
| if len(keyErrs) > 0 { | ||
| err := s.handleSplitRegionKeyErrors(bo, keyErrs) | ||
| if err != nil { | ||
| batchResp.Error = err | ||
| return batchResp | ||
| } | ||
| resp, err = s.splitBatchRegionsReq(bo, batch.Keys, scatter, tableID) | ||
| batchResp.Response = resp | ||
| batchResp.Error = err | ||
| return batchResp | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the split retry against unbounded recursion.
batchSendSingleRegion calls splitBatchRegionsReq, which calls batchSendSingleRegion again. In the region-error branch (Line 170-180) the code always calls bo.Backoff first, so the backoffer eventually exceeds maxSleep and terminates the recursion. The new key-error branch has no such guarantee. handleSplitRegionKeyErrors only sleeps when resolveLockRes.TTL > 0 (Line 268). If TiKV keeps returning a lock key error while the resolver reports TTL 0, no backoff is recorded, the loop never terminates through the backoffer, and the recursion depth grows without bound.
Record a backoff on every key-error retry.
🛡️ Proposed fix
keyErrs := spResp.GetErrors()
if len(keyErrs) > 0 {
err := s.handleSplitRegionKeyErrors(bo, keyErrs)
if err != nil {
batchResp.Error = err
return batchResp
}
+ if err = bo.Backoff(retry.BoTxnLock, errors.Errorf("split region lockedKeys: %d", len(keyErrs))); err != nil {
+ batchResp.Error = err
+ return batchResp
+ }
resp, err = s.splitBatchRegionsReq(bo, batch.Keys, scatter, tableID)If handleSplitRegionKeyErrors should own the backoff, make its sleep unconditional instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| keyErrs := spResp.GetErrors() | |
| if len(keyErrs) > 0 { | |
| err := s.handleSplitRegionKeyErrors(bo, keyErrs) | |
| if err != nil { | |
| batchResp.Error = err | |
| return batchResp | |
| } | |
| resp, err = s.splitBatchRegionsReq(bo, batch.Keys, scatter, tableID) | |
| batchResp.Response = resp | |
| batchResp.Error = err | |
| return batchResp | |
| } | |
| keyErrs := spResp.GetErrors() | |
| if len(keyErrs) > 0 { | |
| err := s.handleSplitRegionKeyErrors(bo, keyErrs) | |
| if err != nil { | |
| batchResp.Error = err | |
| return batchResp | |
| } | |
| if err = bo.Backoff(retry.BoTxnLock, errors.Errorf("split region lockedKeys: %d", len(keyErrs))); err != nil { | |
| batchResp.Error = err | |
| return batchResp | |
| } | |
| resp, err = s.splitBatchRegionsReq(bo, batch.Keys, scatter, tableID) | |
| batchResp.Response = resp | |
| batchResp.Error = err | |
| return batchResp | |
| } |
🤖 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 `@tikv/split_region.go` around lines 183 - 196, Ensure every key-error retry in
the batch split flow records backoff before recursively calling
splitBatchRegionsReq. Update the key-error branch in batchSendSingleRegion or
make handleSplitRegionKeyErrors perform an unconditional backoff, including when
resolveLockRes.TTL is zero, while preserving existing error handling.
| func (c *twoPhaseCommitter) executeTxnFileAction(bo *retry.Backoffer, chunkSlice txnChunkSlice, action txnFileAction) error { | ||
| for { | ||
| batches, err := chunkSlice.groupToBatches(c.store.GetRegionCache(), bo, c.mutations) | ||
| if err != nil { | ||
| return errors.Wrap(err, "txn file: group to batches failed") | ||
| } | ||
|
|
||
| regionErr, err := c.executeTxnFilePrimaryBatch(bo, batches[0], action) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a length check before indexing batches[0].
groupToBatches returns one batch per region that MutationsHasDataInRange reports as non-empty. It can return an empty slice when no mutation falls inside any chunk range, for example after a regroup race. Line 978 then panics with an index-out-of-range error on the commit path.
🛡️ Proposed guard
batches, err := chunkSlice.groupToBatches(c.store.GetRegionCache(), bo, c.mutations)
if err != nil {
return errors.Wrap(err, "txn file: group to batches failed")
}
+ if len(batches) == 0 {
+ return errors.Errorf("txn file: no batch for action %s, startTS %d", action, c.startTS)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *twoPhaseCommitter) executeTxnFileAction(bo *retry.Backoffer, chunkSlice txnChunkSlice, action txnFileAction) error { | |
| for { | |
| batches, err := chunkSlice.groupToBatches(c.store.GetRegionCache(), bo, c.mutations) | |
| if err != nil { | |
| return errors.Wrap(err, "txn file: group to batches failed") | |
| } | |
| regionErr, err := c.executeTxnFilePrimaryBatch(bo, batches[0], action) | |
| func (c *twoPhaseCommitter) executeTxnFileAction(bo *retry.Backoffer, chunkSlice txnChunkSlice, action txnFileAction) error { | |
| for { | |
| batches, err := chunkSlice.groupToBatches(c.store.GetRegionCache(), bo, c.mutations) | |
| if err != nil { | |
| return errors.Wrap(err, "txn file: group to batches failed") | |
| } | |
| if len(batches) == 0 { | |
| return errors.Errorf("txn file: no batch for action %s, startTS %d", action, c.startTS) | |
| } | |
| regionErr, err := c.executeTxnFilePrimaryBatch(bo, batches[0], action) |
🤖 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.go` around lines 971 - 978, Add an empty-slice
guard in twoPhaseCommitter.executeTxnFileAction after groupToBatches succeeds
and before accessing batches[0]. When no batches are returned, skip the
primary-batch execution path safely and continue or return according to the
surrounding transaction flow, while preserving existing handling for non-empty
batches.
| buf = binary.LittleEndian.AppendUint16(buf, uint16(len(key))) | ||
| buf = append(buf, key...) | ||
| buf = append(buf, byte(op)) | ||
| buf = binary.LittleEndian.AppendUint32(buf, uint32(len(val))) | ||
| buf = append(buf, val...) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject keys longer than 65535 bytes before writing the length prefix.
Line 1079 narrows len(key) to uint16. A key of 65536 bytes or more wraps and writes a wrong length prefix. The chunk uploads without an error, and tikv-worker then parses the entry stream at the wrong offsets. The result is silent chunk corruption rather than a visible failure. The same applies to uint32(len(val)) on Line 1082, although that bound is far less reachable.
Validate the sizes and return an error instead.
🛡️ Proposed fix
entrySize := 2 + len(key) + 1 + 4 + len(val)
+ if len(key) > math.MaxUint16 {
+ return errors.Errorf("txn file: key length %d exceeds %d", len(key), math.MaxUint16)
+ }
+ if len(val) > math.MaxUint32 {
+ return errors.Errorf("txn file: value length %d exceeds %d", len(val), uint64(math.MaxUint32))
+ }Add "math" to the imports for this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buf = binary.LittleEndian.AppendUint16(buf, uint16(len(key))) | |
| buf = append(buf, key...) | |
| buf = append(buf, byte(op)) | |
| buf = binary.LittleEndian.AppendUint32(buf, uint32(len(val))) | |
| buf = append(buf, val...) | |
| if len(key) > math.MaxUint16 { | |
| return errors.Errorf("txn file: key length %d exceeds %d", len(key), math.MaxUint16) | |
| } | |
| if len(val) > math.MaxUint32 { | |
| return errors.Errorf("txn file: value length %d exceeds %d", len(val), uint64(math.MaxUint32)) | |
| } | |
| buf = binary.LittleEndian.AppendUint16(buf, uint16(len(key))) | |
| buf = append(buf, key...) | |
| buf = append(buf, byte(op)) | |
| buf = binary.LittleEndian.AppendUint32(buf, uint32(len(val))) | |
| buf = append(buf, val...) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1081-1081: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(val))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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.go` around lines 1079 - 1083, Validate key and
value lengths before serializing the entry, rejecting lengths above the
representable uint16 and uint32 limits instead of narrowing them. Update the
surrounding serialization method to return descriptive errors for oversized keys
or values, add the math import for the bounds, and only write the length
prefixes after validation succeeds.
Source: Linters/SAST tools
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>
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 |
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> # Conflicts: # txnkv/client.go # txnkv/transaction/txn_file_test.go
1 participant
Summary
Stacked on #1998 (transaction: Support file based transaction). Only the last 3 commits are new; everything else is #1998's diff.
Changes after #1998:
integration_tests_in_docker/: new standalone Go module with a Docker Compose fixture that exercises the txn-file path against a real NextGen cluster: PD, three TiKV stores (storage API v2,local_normalkeyspace),tikv-worker, and MinIO-backed DFS. The in-network test suite proves successful txn-file commits and determinate write-conflict rollback, with lock-cleanup evidence gathered via directtikvrpc.CmdGet. Entry point:./integration_tests_in_docker/docker-compose/run.sh.txnkv.Client.Close: new method that closes the KVStore and releases idle HTTP connections opened for txn-file chunk uploads via the newtransaction.CloseTxnFileIdleConnections.txnkv/transaction/txn_file.go: extract batch-concurrency computation intotxnFileBatchConcurrency; make the shared chunk-upload HTTP client anatomic.Pointerso idle connections can be closed safely while another client is using the uploader.Tests
TestTxnFileBatchConcurrency,TestCloseTxnFileIdleConnections, and anio.Closerconformance check fortxnkv.Client(txnkv/client_test.go).go test -v -count=1 -timeout=10m ./txn_fileinside the Compose network (seeintegration_tests_in_docker/README.md).Notes
Client.Closemethod.Summary by CodeRabbit
New Features
Bug Fixes
Documentation