config(dm): add on-duplicate-logical, on-duplicate-physical #7714

lance6716 · 2022-11-25T02:07:58Z

Signed-off-by: lance6716 lance6716@gmail.com

What problem does this PR solve?

Issue Number: ref #3510

What is changed and how it works?

Check List

Tests

Unit test
Integration test

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

add on-duplicate-logical, on-duplicate-physical for load unit configuration

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2022-11-25T02:08:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

buchuitoudegou
okJiang

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lance6716 · 2022-11-25T02:08:37Z

/run-dm-integration-test

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-25T04:08:30Z

/run-dm-integration-test

Signed-off-by: lance6716 <lance6716@gmail.com>

codecov-commenter · 2022-11-25T05:13:44Z

Codecov Report

Merging #7714 (0121d7e) into master (6d24a57) will decrease coverage by 0.0430%.
The diff coverage is 62.1399%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`66.1810% <5.8823%> (-0.1210%)`	⬇️
dm	`52.1867% <59.7826%> (+0.0014%)`	⬆️
engine	`64.2168% <95.2381%> (+0.2200%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #7714        +/-   ##
================================================
- Coverage   59.9034%   59.8604%   -0.0430%     
================================================
  Files           810        810                
  Lines         93255      93185        -70     
================================================
- Hits          55863      55781        -82     
- Misses        32535      32543         +8     
- Partials       4857       4861         +4

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-25T06:04:46Z

/cc @buchuitoudegou @okJiang

buchuitoudegou

rest lgtm

buchuitoudegou · 2022-11-25T06:23:12Z

dm/config/task.go

-	ImportMode  LoadMode             `yaml:"import-mode" toml:"import-mode" json:"import-mode"`
-	OnDuplicate DuplicateResolveType `yaml:"on-duplicate" toml:"on-duplicate" json:"on-duplicate"`
+	PoolSize   int      `yaml:"pool-size" toml:"pool-size" json:"pool-size"`
+	Dir        string   `yaml:"dir" toml:"dir" json:"dir"`


The dir seems a directory that maintains the export data. Do we need an extra config field for sst files?

Thanks! I'll check it with PM later, maybe we need add more fields in future

And also, lightning will check whether the source-dir and sst-dir are on the same disk (for IO performance sake, I think). I'm wondering if we should check it here

precheck will be ported in another PR

buchuitoudegou · 2022-11-25T06:29:13Z

dm/config/task.go

+	// OnDuplicateNone represents do nothing when meet duplicate row and the task will continue.
+	OnDuplicateNone PhysicalDuplicateResolveType = "none"
+	// OnDuplicateManual represents that task should be paused when meet duplicate row to let user handle it manually.
+	OnDuplicateManual PhysicalDuplicateResolveType = "manual"


I remember we have three duplicate resolutions in lightning:

# Physical Import Mode 设置是否检测和解决重复的记录（唯一键冲突）。 # 目前支持三种解决方法： # - record: 仅将重复记录添加到目的 TiDB 中的 `lightning_task_info.conflict_error_v1` 表中。注意，该方法要求目的 TiKV 的版本为 v5.2.0 或更新版本。如果版本过低，则会启用下面的 'none' 模式。 # - none: 不检测重复记录。该模式是三种模式中性能最佳的，但是可能会导致目的 TiDB 中出现数据不一致的情况。 # - remove: 记录所有的重复记录，和 'record' 模式相似。但是会删除所有的重复记录，以确保目的 TiDB 中的数据状态保持一致。 # duplicate-resolution = 'none'

Are there any reasons we don't support it here?

record is worse than remove in all respects so we delete it in design

So we don't support "remove" in this version?

"manual" is in fact "remove", it will remove the conflict KV first and let user query the recording table to decide to insert which row

buchuitoudegou · 2022-11-25T06:31:28Z

dm/worker/subtask.go

@@ -75,7 +75,7 @@ func createRealUnits(cfg *config.SubTaskConfig, etcdClient *clientv3.Client, wor

 func newLoadUnit(cfg *config.SubTaskConfig, etcdClient *clientv3.Client, workerName string) unit.Unit {
 	// tidb-lightning doesn't support column mapping currently
-	if cfg.ImportMode == config.LoadModeLoader || cfg.OnDuplicate == config.OnDuplicateError || len(cfg.ColumnMappingRules) > 0 {
+	if cfg.ImportMode == config.LoadModeLoader || len(cfg.ColumnMappingRules) > 0 {


I remember we have a NeedUseLightning function. Can we reuse it here?

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-28T03:52:38Z

ptal

lance6716 · 2022-11-29T02:06:55Z

ptal @buchuitoudegou @okJiang

okJiang

9/17 file

dm/config/task.go

okJiang · 2022-11-29T02:42:39Z

dm/config/task.go

+	Dir        string   `yaml:"dir" toml:"dir" json:"dir"`
+	SQLMode    string   `yaml:"-" toml:"-" json:"-"` // wrote by dump unit (DM op) or jobmaster (DM in engine)
+	ImportMode LoadMode `yaml:"import-mode" toml:"import-mode" json:"import-mode"`
+	// deprecated, use OnDuplicateLogical instead.


do we have any plan to remove the deprecated configuration?

after all development is finished

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-29T04:09:12Z

/merge

ti-chi-bot · 2022-11-29T04:09:14Z

This pull request has been accepted and is ready to merge.

Commit hash: 34911a8

ti-chi-bot · 2022-11-29T04:09:27Z

@lance6716: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

config(dm): add on-duplicate-logical, on-duplicate-physical

ad627ce

Signed-off-by: lance6716 <lance6716@gmail.com>

fix some test

e07dd6a

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2022

fix lint

7941977

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2022

lance6716 added 2 commits November 25, 2022 13:37

add UT

94fbe89

Signed-off-by: lance6716 <lance6716@gmail.com>

sync code

393f7c0

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 changed the title ~~[WIP]config(dm): add on-duplicate-logical, on-duplicate-physical~~ config(dm): add on-duplicate-logical, on-duplicate-physical Nov 25, 2022

ti-chi-bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Nov 25, 2022

sync code

56dbede

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot requested review from buchuitoudegou and okJiang November 25, 2022 06:04

buchuitoudegou reviewed Nov 25, 2022

View reviewed changes

address comment

0121d7e

Signed-off-by: lance6716 <lance6716@gmail.com>

okJiang reviewed Nov 29, 2022

View reviewed changes

buchuitoudegou approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 29, 2022

address comment

34911a8

Signed-off-by: lance6716 <lance6716@gmail.com>

okJiang approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 29, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

Merge branch 'master' into on-dup

11a032f

ti-chi-bot merged commit 7266c18 into pingcap:master Nov 29, 2022

lance6716 mentioned this pull request Nov 29, 2022

pool-size = 0 will block DM task #7664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config(dm): add on-duplicate-logical, on-duplicate-physical #7714

config(dm): add on-duplicate-logical, on-duplicate-physical #7714

lance6716 commented Nov 25, 2022 •

edited

Loading

ti-chi-bot commented Nov 25, 2022 •

edited

Loading

lance6716 commented Nov 25, 2022

lance6716 commented Nov 25, 2022

codecov-commenter commented Nov 25, 2022 •

edited

Loading

lance6716 commented Nov 25, 2022

buchuitoudegou left a comment

buchuitoudegou Nov 25, 2022

lance6716 Nov 25, 2022

buchuitoudegou Nov 25, 2022

lance6716 Nov 25, 2022

buchuitoudegou Nov 25, 2022

lance6716 Nov 25, 2022

buchuitoudegou Nov 25, 2022

buchuitoudegou Nov 25, 2022

lance6716 Nov 25, 2022

buchuitoudegou Nov 25, 2022

lance6716 Nov 25, 2022

lance6716 commented Nov 28, 2022

lance6716 commented Nov 29, 2022

okJiang left a comment

okJiang Nov 29, 2022

lance6716 Nov 29, 2022

lance6716 commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

config(dm): add on-duplicate-logical, on-duplicate-physical #7714

config(dm): add on-duplicate-logical, on-duplicate-physical #7714

Conversation

lance6716 commented Nov 25, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented Nov 25, 2022 • edited Loading

lance6716 commented Nov 25, 2022

lance6716 commented Nov 25, 2022

codecov-commenter commented Nov 25, 2022 • edited Loading

Codecov Report

lance6716 commented Nov 25, 2022

buchuitoudegou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Nov 28, 2022

lance6716 commented Nov 29, 2022

okJiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

lance6716 commented Nov 25, 2022 •

edited

Loading

ti-chi-bot commented Nov 25, 2022 •

edited

Loading

codecov-commenter commented Nov 25, 2022 •

edited

Loading