lightning: add complex integration tests for lightning post-import conflict detection "replace" mode #47460

lyzx2001 · 2023-10-08T09:55:46Z

What problem does this PR solve?

Issue Number: ref #45774

Problem Summary:

Currently lightning only supports "remove" mode for post-import conflict detection, but many customers request lightning to support "replace" mode for lightning post-import conflict detection.

We would like to support "replace" mode for lightning post-import conflict detection:
To resolve rows with conflict, instead of deleting all the rows that are engaged in conflict (the algorithm for remove), we delete some of the rows with conflict and reserve other rows that can be kept and not cause conflict anymore. Under this circumstance, we only delete the necessary rows to resolve conflicts, so that we can keep more original rows than remove mode as long as the conflicts are resolved.

The algorithms for index KV checking is contained in #45926

The algorithms for data KV checking is contained in #46763

This PR contains complex integration tests to test the correctness and performance of the algorithms.

The tests vary in the setting of engine batch-size to cover various complex cases .

What is changed and how it works?

Demo code for 'replace' mode of lightning post-import conflict detection:

https://github.com/lyzx2001/tidb-conflict-replace

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

…ost-import conflict detection

merge master

…port conflict detection 'replace' mode

tiprow · 2023-10-08T09:56:06Z

Hi @lyzx2001. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions 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 kubernetes/test-infra repository.

codecov · 2023-10-08T10:07:32Z

Codecov Report

Merging #47460 (26f5578) into master (91a8023) will increase coverage by 3.9665%.
Report is 155 commits behind head on master.
The diff coverage is 81.3559%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47460        +/-   ##
================================================
+ Coverage   71.8856%   75.8521%   +3.9664%     
================================================
  Files          1398       1447        +49     
  Lines        405057     418804     +13747     
================================================
+ Hits         291178     317672     +26494     
+ Misses        94273      81140     -13133     
- Partials      19606      19992       +386

Flag	Coverage Δ
integration	`49.5630% <81.3559%> (?)`
unit	`72.2566% <57.6271%> (+0.3709%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9874% <ø> (-0.0039%)`	⬇️
parser	`∅ <ø> (∅)`
br	`67.3197% <81.3559%> (+14.3386%)`	⬆️

lyzx2001 · 2023-10-09T07:51:42Z

/test pull-br-integration-test

tiprow · 2023-10-09T07:52:11Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-br-integration-test

Instructions 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 kubernetes/test-infra repository.

lyzx2001 · 2023-10-09T08:02:18Z

/test pull-br-integration-test

tiprow · 2023-10-30T07:08:37Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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 kubernetes/test-infra repository.

lyzx2001 · 2023-10-30T08:10:49Z

/retest

tiprow · 2023-10-30T08:11:11Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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 kubernetes/test-infra repository.

D3Hunter

rest lgtm

D3Hunter · 2023-11-01T03:05:27Z

br/pkg/lightning/errormanager/errormanager.go

@@ -94,6 +95,7 @@ const (
 			raw_value   mediumblob NOT NULL COMMENT 'the value of the conflicted key',
 			raw_handle  mediumblob NOT NULL COMMENT 'the data handle derived from the conflicted key or value',
 			raw_row     mediumblob NOT NULL COMMENT 'the data retrieved from the handle',
+			is_data_kv  tinyint(1) NOT NULL,


maybe kv_group, values can be data/index, or if we want finer grain, we can use index id as the name.

current is lgtm too

maybe current is_data_kv is simpler in implemention, it seems like it is not necessary to determine whether the value is data or index based on the result tablecodec.IsRecordKey(conflictInfo.RawKey)

D3Hunter · 2023-11-01T03:06:27Z

...htning_duplicate_resolution_replace_multiple_keys_clustered_pk/data/dup_resolve.a-schema.sql

+    c int not null,
+    d text,
+    key key_b(b),
+    key key_c(b)


index on c?

D3Hunter · 2023-11-01T03:09:40Z

br/tests/lightning_duplicate_resolution_replace_multiple_keys_clustered_pk/run.sh

+check_contains 'a: 1'
+check_contains 'b: 1'
+check_contains 'c: 1'
+check_contains 'd: 1.csv'


more like ignore semantic in this case

I did not quite understand. Here 1,1,1,1.csv and 1,1,2,2.csv have conflicts due to PK, and here in replace semantic we reserve the first row and delete the second row.

ignore will keep the first row met, like what's done here, replace will keep the last

Here batch-size = 1, the actual import order is not the same as that presented in dup_resolve.a.1

D3Hunter · 2023-11-01T03:10:17Z

...ing_duplicate_resolution_replace_multiple_keys_nonclustered_pk/data/dup_resolve.a-schema.sql

+    c int not null,
+    d text,
+    key key_b(b),
+    key key_c(b)


D3Hunter · 2023-11-01T03:13:27Z

/lgtm

ti-chi-bot · 2023-11-01T03:13:40Z

[LGTM Timeline notifier]

Timeline:

2023-10-30 06:07:48.546654075 +0000 UTC m=+2846866.133764221: ☑️ agreed by lance6716.
2023-11-01 03:13:39.747115176 +0000 UTC m=+3009217.334225338: ☑️ agreed by D3Hunter.

lyzx2001 · 2023-11-01T05:07:29Z

/test pull-br-integration-test

tiprow · 2023-11-01T05:07:50Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-br-integration-test

Instructions 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 kubernetes/test-infra repository.

wuhuizuo · 2023-11-01T05:17:58Z

go.mod

@@ -186,7 +186,7 @@ require (
 	github.com/eapache/go-resiliency v1.2.0 // indirect
 	github.com/eapache/go-xerial-snappy v0.0.0-20180814174437-776d5712da21 // indirect
 	github.com/eapache/queue v1.1.0 // indirect
-	github.com/fatih/structtag v1.2.0 // indirect
+	github.com/fatih/structtag v1.2.0


Does it become a direct dependency go module?

This deletion was auto performed by make tidy

wuhuizuo · 2023-11-01T05:19:06Z

/review default

ti-chi-bot · 2023-11-01T05:19:52Z

@wuhuizuo:
Potential Problems:

In the config.go file, the comments for DupeResAlgRecord and DupeResAlgRemove still mention the old lightning_task_info.conflict_error_v1 table instead of the new lightning_task_info.conflict_error_v2 table.

Suggestions:

Update the comments for DupeResAlgRecord and DupeResAlgRemove to mention the new lightning_task_info.conflict_error_v2 table instead of the old lightning_task_info.conflict_error_v1 table.

diff --git a/br/pkg/lightning/config/config.go b/br/pkg/lightning/config/config.go
index 6d71f35795b923..03d97522a4ebd2 100644
--- a/br/pkg/lightning/config/config.go
+++ b/br/pkg/lightning/config/config.go
@@ -588,16 +588,16 @@ const (
 	// DupeResAlgNone doesn't detect duplicate.
 	DupeResAlgNone DuplicateResolutionAlgorithm = iota
 
-	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v1` table on the target TiDB.
+	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v2` table on the target TiDB.
 	DupeResAlgRecord
 
 	// DupeResAlgRemove records all duplicate records like the 'record' algorithm and remove all information related to the
-	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v1 table to add back the correct rows.
+	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v2 table to add back the correct rows.
 	DupeResAlgRemove
 
 	// DupeResAlgReplace records all duplicate records like the 'record' algorithm, and remove some rows with conflict
 	// and reserve other rows that can be kept and not cause conflict anymore. Users need to analyze the
-	// lightning_task_info.conflict_error_v1 table to check whether the reserved data cater to their need and check whether
+	// lightning_task_info.conflict_error_v2 table to check whether the reserved data cater to their need and check whether
 	// they need to add back the correct rows

......

Response is trunked for length limits.

In response to this:

/review default

Instructions 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 kubernetes/test-infra repository.

lyzx2001 · 2023-11-01T06:27:07Z

@wuhuizuo: Potential Problems:

In the config.go file, the comments for DupeResAlgRecord and DupeResAlgRemove still mention the old lightning_task_info.conflict_error_v1 table instead of the new lightning_task_info.conflict_error_v2 table.

Suggestions:

Update the comments for DupeResAlgRecord and DupeResAlgRemove to mention the new lightning_task_info.conflict_error_v2 table instead of the old lightning_task_info.conflict_error_v1 table.

diff --git a/br/pkg/lightning/config/config.go b/br/pkg/lightning/config/config.go
index 6d71f35795b923..03d97522a4ebd2 100644
--- a/br/pkg/lightning/config/config.go
+++ b/br/pkg/lightning/config/config.go
@@ -588,16 +588,16 @@ const (
 	// DupeResAlgNone doesn't detect duplicate.
 	DupeResAlgNone DuplicateResolutionAlgorithm = iota
 
-	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v1` table on the target TiDB.
+	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v2` table on the target TiDB.
 	DupeResAlgRecord
 
 	// DupeResAlgRemove records all duplicate records like the 'record' algorithm and remove all information related to the
-	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v1 table to add back the correct rows.
+	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v2 table to add back the correct rows.
 	DupeResAlgRemove
 
 	// DupeResAlgReplace records all duplicate records like the 'record' algorithm, and remove some rows with conflict
 	// and reserve other rows that can be kept and not cause conflict anymore. Users need to analyze the
-	// lightning_task_info.conflict_error_v1 table to check whether the reserved data cater to their need and check whether
+	// lightning_task_info.conflict_error_v2 table to check whether the reserved data cater to their need and check whether
 	// they need to add back the correct rows

......

Response is trunked for length limits.

In the code all lightning_task_info.conflict_error_v1 have already been changed to lightning_task_info.conflict_error_v2, we may ignore this suggestion.

lyzx2001 · 2023-11-02T03:59:30Z

/cc @3pointer

wuhuizuo · 2023-11-02T06:19:29Z

@wuhuizuo: Potential Problems:

In the config.go file, the comments for DupeResAlgRecord and DupeResAlgRemove still mention the old lightning_task_info.conflict_error_v1 table instead of the new lightning_task_info.conflict_error_v2 table.

Suggestions:

Update the comments for DupeResAlgRecord and DupeResAlgRemove to mention the new lightning_task_info.conflict_error_v2 table instead of the old lightning_task_info.conflict_error_v1 table.

diff --git a/br/pkg/lightning/config/config.go b/br/pkg/lightning/config/config.go
index 6d71f35795b923..03d97522a4ebd2 100644
--- a/br/pkg/lightning/config/config.go
+++ b/br/pkg/lightning/config/config.go
@@ -588,16 +588,16 @@ const (
 	// DupeResAlgNone doesn't detect duplicate.
 	DupeResAlgNone DuplicateResolutionAlgorithm = iota
 
-	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v1` table on the target TiDB.
+	// DupeResAlgRecord only records duplicate records to `lightning_task_info.conflict_error_v2` table on the target TiDB.
 	DupeResAlgRecord
 
 	// DupeResAlgRemove records all duplicate records like the 'record' algorithm and remove all information related to the
-	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v1 table to add back the correct rows.
+	// duplicated rows. Users need to analyze the lightning_task_info.conflict_error_v2 table to add back the correct rows.
 	DupeResAlgRemove
 
 	// DupeResAlgReplace records all duplicate records like the 'record' algorithm, and remove some rows with conflict
 	// and reserve other rows that can be kept and not cause conflict anymore. Users need to analyze the
-	// lightning_task_info.conflict_error_v1 table to check whether the reserved data cater to their need and check whether
+	// lightning_task_info.conflict_error_v2 table to check whether the reserved data cater to their need and check whether
 	// they need to add back the correct rows

......

Response is trunked for length limits.

In the code all lightning_task_info.conflict_error_v1 have already been changed to lightning_task_info.conflict_error_v2, we may ignore this suggestion.

Yes, the AI misjudged it.

hawkingrei · 2023-11-03T06:53:11Z

/ok-to-test

lyzx2001 · 2023-11-03T07:47:44Z

/cc @easonn7

ti-chi-bot · 2023-11-03T07:47:47Z

@lyzx2001: GitHub didn't allow me to request PR reviews from the following users: easonn7.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @easonn7

Instructions 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 kubernetes/test-infra repository.

easonn7 · 2023-11-03T08:09:03Z

/approve

ti-chi-bot · 2023-11-03T08:09:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, D3Hunter, easonn7, lance6716

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [3pointer,D3Hunter,lance6716]
~~br/OWNERS~~ [3pointer,easonn7]
~~br/pkg/lightning/OWNERS~~ [D3Hunter,lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lyzx2001 · 2023-11-03T08:10:15Z

/retest

lyzx2001 added 15 commits September 7, 2023 22:21

lightning: support data KV checking of 'replace' mode for lightning p…

61218e7

…ost-import conflict detection

update unit tests

4ac742a

update unit tests

e653151

update unit tests

f881845

update according to comments

3075acc

add more INDEX

bb1ddd4

update algorithms of checking data KV

7d55ced

Merge branch 'master' into issue45774-step3

30cfa1e

merge master

update createConflictErrorTable

19ec0e4

update error checking

614f92a

add some comments

d0b86df

Merge branch 'master' into issue45774-step3

4462832

merge master

add unit tests

a2f1414

lightning: add unit tests and integration tests for lightning post-im…

5429f4e

…port conflict detection 'replace' mode

merge master

e2d0c8b

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 8, 2023

modify unit tests

b59dc5f

lyzx2001 changed the title ~~[WIP] lightning: add complex unit tests and integration tests for lightning post-import conflict detection~~ [WIP] lightning: add complex unit tests and integration tests (part 1) for lightning post-import conflict detection Oct 9, 2023

add multiple unit tests and integration tests

4269ff3

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 9, 2023

lyzx2001 changed the title ~~[WIP] lightning: add complex unit tests and integration tests (part 1) for lightning post-import conflict detection~~ [WIP] lightning: add complex unit tests and integration tests (part 1) for lightning post-import conflict detection "replace" mode Oct 9, 2023

modify run_group.sh

2a1cef4

D3Hunter reviewed Nov 1, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 1, 2023

update IT

26f5578

wuhuizuo reviewed Nov 1, 2023

View reviewed changes

lyzx2001 requested a review from D3Hunter November 1, 2023 06:27

ti-chi-bot bot requested a review from 3pointer November 2, 2023 03:59

3pointer approved these changes Nov 3, 2023

View reviewed changes

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 3, 2023

ti-chi-bot bot added the approved label Nov 3, 2023

ti-chi-bot bot merged commit 37965e5 into pingcap:master Nov 3, 2023
8 checks passed

lyzx2001 mentioned this pull request Nov 10, 2023

lightning: add complex unit tests for lightning post-import conflict detection "replace" mode #48514

Merged

13 tasks

lightning: add complex integration tests for lightning post-import conflict detection "replace" mode #47460

lightning: add complex integration tests for lightning post-import conflict detection "replace" mode #47460

Conversation

lyzx2001 commented Oct 8, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Oct 8, 2023

codecov bot commented Oct 8, 2023 • edited Loading

Codecov Report

lyzx2001 commented Oct 9, 2023

tiprow bot commented Oct 9, 2023

lyzx2001 commented Oct 9, 2023

tiprow bot commented Oct 30, 2023

lyzx2001 commented Oct 30, 2023

tiprow bot commented Oct 30, 2023

D3Hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

[LGTM Timeline notifier]

lyzx2001 commented Nov 1, 2023

tiprow bot commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuhuizuo commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023 • edited by wuhuizuo Loading

lyzx2001 commented Nov 1, 2023

lyzx2001 commented Nov 2, 2023

wuhuizuo commented Nov 2, 2023 • edited Loading

hawkingrei commented Nov 3, 2023

lyzx2001 commented Nov 3, 2023

ti-chi-bot bot commented Nov 3, 2023

easonn7 commented Nov 3, 2023

ti-chi-bot bot commented Nov 3, 2023

lyzx2001 commented Nov 3, 2023

lyzx2001 commented Oct 8, 2023 •

edited

Loading

codecov bot commented Oct 8, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 1, 2023 •

edited by wuhuizuo

Loading

wuhuizuo commented Nov 2, 2023 •

edited

Loading