[import] conflict traffic metering miscounts cache hits and API V2 key prefixes
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
Conflict-resolution cluster traffic metering is inaccurate in two independent ways:
1. `LazyRefreshedSnapshot.BatchGet` adds `len(key) + len(value)` for every returned entry, including entries that client-go serves entirely from the snapshot cache without a TiKV RPC.
2. In NextGen/API V2, client-go exposes logical keys after codec decoding, while TiKV receives physical keys with a four-byte keyspace prefix. Conflict read and delete accounting uses the logical lengths and therefore omits four bytes per key. Classic/API V1 does not have this prefix discrepancy.
This is metering correctness only. It does not corrupt SQL rows, indexes, or conflict resolution results. `TrafficRecorder` is explicitly documented as best-effort, so this report is about making that best-effort estimate track actual cluster traffic more closely.
### 1. Minimal reproduce step (Required)
#### Test A: a repeated `BatchGet` is a cache hit but still grows the recorder
Add a deterministic unit test around `LazyRefreshedSnapshot` using one existing key `K` with value `V`:
1. Create one `LazyRefreshedSnapshot` with a counting `TrafficRecorder`.
2. Call `BatchGet(ctx, []Key{K})` once. This reads the entry and populates client-go's snapshot cache.
3. Record the traffic counter, install/count a client-go RPC interceptor, and call `BatchGet` again with the same key before the 15-second snapshot refresh interval expires.
4. Assert that the second call returns `K -> V`, issues zero `CmdBatchGet` RPCs, and inspect the recorder delta.
client-go's pinned `KVSnapshot.BatchGetWithTier` removes cache hits from the keys sent to the region/RPC path and returns immediately when no keys remain. TiDB nevertheless iterates over the returned map and records it again.
**Expected:** the second call's cluster-read byte delta is `0`, because the value is served from the local snapshot cache and no cluster read occurs.
**Actual at the audited HEAD:** the second call's cluster-read byte delta is `len(K) + len(V)` even though the intercepted `CmdBatchGet` RPC count is `0`. Repeating the call repeats the overcount.
#### Test B: API V2 physical-key lengths include four bytes that the recorder cannot see
Add a separate deterministic NextGen/API V2 test using a known logical key `K`, value `V`, and a client-go V2 transaction codec for a fixed keyspace ID:
1. Assert that `len(codec.EncodeKey(K)) == len(K) + 4`.
2. Read `K` through `LazyRefreshedSnapshot.BatchGet`. The response map key exposed after client-go response decoding is the logical `K`; compare the recorded bytes with the physical encoded-key length.
3. Delete the same logical key through `Deleter`. Compare the recorded write bytes with the V2-encoded delete-mutation key length.
4. Run the length assertions separately from Test A so the cache-hit result cannot affect this case.
**Expected in NextGen/API V2:** a returned pair contributes `len(codec.EncodeKey(K)) + len(V)`, or `len(K) + 4 + len(V)`, to cluster-read bytes. A delete contributes `len(codec.EncodeKey(K))`, or `len(K) + 4`, to cluster-write bytes.
**Actual at the audited HEAD:** the read recorder adds only `len(K) + len(V)`, and the delete recorder adds only `len(K)`. Both undercount by four bytes for every physical key. Classic/API V1 is unaffected by this prefix-specific case.
Relevant code and pinned dependency behavior:
- [`LazyRefreshedSnapshot.BatchGet` counts every returned logical entry](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L356-L377).
- [`Deleter` counts logical delete-key lengths before the transaction writes them](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter.go#L151-L178).
- [`TrafficRecorder` describes this traffic as best-effort](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L49-L55), and the concrete recorder stores the totals as cluster traffic ([implementation](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/framework/metering/recorder.go#L34-L42)).
- The audited [`go.mod` pins client-go](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/go.mod#L125) to `v2.0.8-0.20260708122311-01bd8f99f4da`.
- At that client-go revision, [`BatchGetWithTier` returns cached values without entering the RPC path when all keys hit](https://github.com/tikv/client-go/blob/01bd8f99f4da/txnkv/txnsnapshot/snapshot.go#L262-L291), and [`UpdateSnapshotCache` stores results for later calls](https://github.com/tikv/client-go/blob/01bd8f99f4da/txnkv/txnsnapshot/snapshot.go#L1133-L1152).
- Its API V2 codec defines a [four-byte keyspace prefix](https://github.com/tikv/client-go/blob/01bd8f99f4da/internal/apicodec/codec_v2.go#L26-L31), encodes physical keys by prepending it, and removes it when decoding ([codec methods](https://github.com/tikv/client-go/blob/01bd8f99f4da/internal/apicodec/codec_v2.go#L799-L820)).
Static-audit disclaimer: this report was derived from the code paths above. The proposed regression tests have not been added or executed as part of this audit, and no end-to-end billing impact was measured.
### 2. What did you expect to see? (Required)
Conflict-handling cluster traffic should count bytes attributable to actual TiKV reads/writes: cached `BatchGet` results should not add cluster-read bytes again, and API V2 accounting should use the physical encoded-key length including the four-byte keyspace prefix.
### 3. What did you see instead (Required)
Repeated reads can increase `clusterReadBytes` without a corresponding RPC, while NextGen/API V2 reads and deletes undercount four bytes for each physical key. Depending on access patterns, the two read errors can partially offset or dominate one another; they should be tested and fixed independently.
### 4. What is your TiDB version? (Required)
Audited source HEAD: `59f6e85cd01756d53f799c33afba0a185d956d3f`.
Pinned client-go: `github.com/tikv/client-go/v2 v2.0.8-0.20260708122311-01bd8f99f4da`.
The static evidence does not establish an affected released TiDB version, so no `affects-*` label is requested.
Contributor guide
Research direction
Start with pkg/dxf/importinto/conflictedkv/handler.go and deleter.go, then inspect the pinned client-go snapshot and API V2 codec behavior referenced in the report. Add separate deterministic tests for repeated cached BatchGet calls and NextGen/API V2 key lengths. Done means cache hits add zero cluster-read bytes and V2 read/delete accounting matches physical encoded-key lengths, without changing conflict results.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100