importinto: fix conflict row identity and MVI deduplication (#70076) - #70208
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@D3Hunter This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConflict collection and resolution now identify rows by physical row keys instead of handles, decode keys through storage codecs, dispatch MV-index pairs to dedicated workers, and retry transaction-retryable deletions. Tests cover bounded tracking, cancellation, codec variants, multi-valued indexes, partitioned global indexes, and conflicting common handles. ChangesConflict Processing Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dxf/importinto/conflictedkv/handler.go (1)
110-126: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
NewBaseHandlerwas resolved to the old signature while callers use the new one, andcollectoris never assigned.
BaseHandlernow declarescollector execute.Collector, but this constructor neither accepts nor sets it, so progress reporting is dead. Meanwhilecollector.go(Line 134) andhandler_test.go(Lines 115-117, 199-202, 306-314) pass a 7thprogressCollectorargument. Add the parameter and wire it.🐛 Proposed fix
func NewBaseHandler( targetTable table.Table, kvGroup string, codec tikv.Codec, encoder *importer.TableKVEncoder, encodedRowHdl EncodedRowHandler, + collector execute.Collector, logger *zap.Logger, ) *BaseHandler { return &BaseHandler{ targetTable: targetTable, kvGroup: kvGroup, codec: codec, encoder: encoder, + collector: collector, logger: logger, EncodedRowHandler: encodedRowHdl, } }🤖 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 `@pkg/dxf/importinto/conflictedkv/handler.go` around lines 110 - 126, Update NewBaseHandler to accept the execute.Collector progressCollector argument expected by its callers, and assign it to the returned BaseHandler’s collector field. Preserve the existing initialization of the other constructor fields so progress reporting is wired without changing unrelated behavior.
🧹 Nitpick comments (1)
tests/realtikvtest/importintotest4/conflict_resolution_test.go (1)
521-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the surviving rows for the common-handle regression.
The other two new tests verify final table state; this one only asserts an empty conflicted-row result, so a regression that deletes the wrong handle could still pass. A
MustQuery("select * from t ...")check on the expected remaining rows would pin the intent of issue#69801.🤖 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 `@tests/realtikvtest/importintotest4/conflict_resolution_test.go` around lines 521 - 541, Extend TestGlobalSortDistinguishesCommonHandlesWithSameString to assert the final contents of table t after conflict resolution, including the expected surviving rows and ordering. Keep the existing empty conflicted-row assertion, and use the established query assertion helper to verify the regression preserves the correct handles from the two distinct conflict groups.
🤖 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 `@pkg/dxf/importinto/conflictedkv/collector_test.go`:
- Around line 29-33: Resolve all merge conflicts in
pkg/dxf/importinto/conflictedkv/collector_test.go:29-33 by retaining imports
required by the resolved API, choose the matching NewCollector constructor calls
at 179-183, make the collector configuration consistent at 267-271, and resolve
both collector initializations consistently at 365-377; remove every conflict
marker so the test file parses.
In `@pkg/dxf/importinto/conflictedkv/deleter.go`:
- Around line 84-88: Resolve the cherry-pick conflict as one coherent API
migration: in pkg/dxf/importinto/conflictedkv/deleter.go lines 84-88, remove the
conflict markers and use the resolved NewBaseHandler contract consistently. In
pkg/dxf/importinto/conflictedkv/deleter_test.go lines 24-32, 139-144, 156-162,
184-200, and 205-223, align imports, NewDeleter arguments, encoded KV channel
types, and KV-group constants with the resolved external/simplesst helper and
deleter.Run APIs, including index-conflict coverage.
In `@pkg/dxf/importinto/conflictedkv/handler.go`:
- Around line 22-27: Remove all unresolved conflict markers and retain the
cherry-pick resolution across the affected sites: in
pkg/dxf/importinto/conflictedkv/handler.go (22-27, 90-103, 197-203, 274-280),
keep execute and the codec/simplesst.KVPair implementations; in
pkg/dxf/importinto/conflictedkv/collector.go (131-135, 161-175), keep the
codec-aware NewBaseHandler call and remove hdlSet.Add; in
pkg/dxf/importinto/conflictedkv/handler_test.go (111-118, 195-203, 254-265),
keep globalsort and execute.TestCollector with the matching resolution. In
pkg/dxf/importinto/collect_conflicts.go (37-41, 184-192), keep
globalsort/simplesst and getKVGroupIndexInfo; in
pkg/dxf/importinto/conflict_resolution.go (143-148), keep
globalsort.ReadKVFilesAsync and createConflictHandlerChannels; in
pkg/dxf/importinto/BUILD.bazel (116-120, 166-170), keep shard_count 37, resolve
dependencies accordingly, and regenerate with make bazel_prepare.
---
Outside diff comments:
In `@pkg/dxf/importinto/conflictedkv/handler.go`:
- Around line 110-126: Update NewBaseHandler to accept the execute.Collector
progressCollector argument expected by its callers, and assign it to the
returned BaseHandler’s collector field. Preserve the existing initialization of
the other constructor fields so progress reporting is wired without changing
unrelated behavior.
---
Nitpick comments:
In `@tests/realtikvtest/importintotest4/conflict_resolution_test.go`:
- Around line 521-541: Extend
TestGlobalSortDistinguishesCommonHandlesWithSameString to assert the final
contents of table t after conflict resolution, including the expected surviving
rows and ordering. Keep the existing empty conflicted-row assertion, and use the
established query assertion helper to verify the regression preserves the
correct handles from the two distinct conflict groups.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97586dce-3e8b-4cda-8b45-46a09fc758e2
📒 Files selected for processing (17)
docs/agents/import-into/README.mdpkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/collect_conflicts.gopkg/dxf/importinto/collect_conflicts_internal_test.gopkg/dxf/importinto/conflict_resolution.gopkg/dxf/importinto/conflictedkv/BUILD.bazelpkg/dxf/importinto/conflictedkv/collector.gopkg/dxf/importinto/conflictedkv/collector_test.gopkg/dxf/importinto/conflictedkv/deleter.gopkg/dxf/importinto/conflictedkv/deleter_internal_test.gopkg/dxf/importinto/conflictedkv/deleter_test.gopkg/dxf/importinto/conflictedkv/doc.gopkg/dxf/importinto/conflictedkv/handler.gopkg/dxf/importinto/conflictedkv/handler_test.gopkg/dxf/importinto/conflictedkv/row_handle.gopkg/dxf/importinto/conflictedkv/row_handle_test.gotests/realtikvtest/importintotest4/conflict_resolution_test.go
💤 Files with no reviewable changes (1)
- pkg/dxf/importinto/conflictedkv/BUILD.bazel
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #70208 +/- ##
===========================================================
Coverage ? 76.2610%
===========================================================
Files ? 1937
Lines ? 541444
Branches ? 0
===========================================================
Hits ? 412911
Misses ? 128533
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, joechenrh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
6b91e9d
into
pingcap:release-nextgen-202603
3 participants
This is an automated cherry-pick of #70076
What problem does this PR solve?
Issue Number: close #69799, close #69800, close #69801
Problem Summary:
Global-sort
IMPORT INTO ... WITH on_duplicate_key='capture'can identify conflicted rows incorrectly in three related cases:Handle.String()as row identity. Distinct common handles can have the same display string, and partition handles omit the partition ID from that string, causing valid rows to be skipped.These cases can produce an incorrect conflicted-row count or checksum, fail post-process checksum validation, or leave inconsistent row and index KVs after conflict resolution.
What changed and how does it work?
tablecodec.EncodeRecordKeyhonorsPartitionHandle.PartitionID, while the binary row key avoids collisions between distinct common handles.Check List
Tests
PR #70076 is effectively unchanged for the four-plain-UK workload, with only a 0.01% total-time difference from master. It shows a modest 3.14% slowdown for the single-MV-index-KV workload, including a 9.30% increase in
collect-conflictstime.For the 15M rows, 45M conflicted KVs, three MV index KVs per row workload, PR #70076 is 4.37% faster overall, mainly because
collect-conflictsis 22.08% faster. However, the master run is not a successful baseline: it failed with the expectedchecksum mismatch. On master, the MV index KVs associated with each row are handled three times, and write conflicts occur while deleting them. This additional work makes master slightly slower than the PR, particularly during conflict resolution, where the PR is 3.300 seconds, or 0.57%, faster.Overall, the results do not show a consistent performance regression from the new KV routing. The PR has a small penalty in the 15M-KV case but improves the heavier 45M-KV case. Because the master run for the 45M-KV scenario failed and performed extra conflict-handling work, that result should be treated as a diagnostic time comparison rather than a like-for-like successful end-to-end comparison.
Commands run:
The RealTiKV case used an isolated
tiup playground --mode tikv-slim --tag realtikvtest --port-offset 10000, and the PD endpoint was verified unreachable after teardown.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit