dm: new router compatible with regular expression #4358

buchuitoudegou · 2022-01-17T07:26:13Z

What problem does this PR solve?

implement new router that is compatible with regular expressions

Issue Number: close #4256

What is changed and how it works?

A new router that is compatible with regular expressions is implemented. It basically reused the filter module in tidb-tools project, which has also been used in the block-allow-list. Reusing the existing module ensures modules behaving the same across the whole project.

Check List

Tests

Unit test
Integration test

Code changes

New router implemented as a stand-alone module and exporting the same interface with the old one
Replace the old router with the new one

Side effects

Related changes

Release note

Implement a new router that is compatible with regular expression as well as the old router

ti-chi-bot · 2022-01-17T07:26:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

GMHDBJD
lance6716

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.

CLAassistant · 2022-01-17T07:26:18Z

All committers have signed the CLA.

codecov-commenter · 2022-01-18T07:42:21Z

Codecov Report

Merging #4358 (85dd8ee) into master (9607554) will increase coverage by 0.0075%.
The diff coverage is 62.5546%.

Flag	Coverage Δ
cdc	`60.3175% <59.3750%> (+0.3953%)`	⬆️
dm	`51.6932% <63.4388%> (-0.3356%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #4358        +/-   ##
================================================
+ Coverage   55.6402%   55.6477%   +0.0075%     
================================================
  Files           494        507        +13     
  Lines         61283      62768      +1485     
================================================
+ Hits          34098      34929       +831     
- Misses        23750      24355       +605     
- Partials       3435       3484        +49

dm/regexprrouter/router.go

buchuitoudegou · 2022-01-21T03:37:40Z

/run-kafka-integration-test /tidb=pr/30558

buchuitoudegou · 2022-01-21T03:37:56Z

/run-integration-test /tidb=pr/30558

Ehco1996 · 2022-01-24T01:28:19Z

dm/dm/config/subtask.go

@@ -25,12 +25,12 @@ import (
 	bf "github.com/pingcap/tidb-tools/pkg/binlog-filter"
 	"github.com/pingcap/tidb-tools/pkg/column-mapping"
 	"github.com/pingcap/tidb-tools/pkg/filter"
-	router "github.com/pingcap/tidb-tools/pkg/table-router"


just a question: why not pull a request to tidb-tools?
maybe i miss some offline talk infomation😂

No, nice suggestion. We're planning moving it to tidb-tools to keep the table router aligned across all the project.

dm/tests/all_mode/run.sh

dm/pkg/router/router_test.go

GMHDBJD

LGTM
Please resolve conflict

lance6716

rest lgtm

dm/pkg/router/router.go

dm/pkg/router/router_test.go

lance6716 · 2022-02-10T03:19:13Z

dm/pkg/router/router_test.go

+		{
+			SchemaPattern: "~s?chema.*",
+			TargetSchema:  "test",
+			TargetTable:   "t2",


this usage (no table-pattern but target-table) is strange, should we forbid it in future? @GMHDBJD @lichunzhu

IMO we should forbid this kind of behavior

What if the users would like to migrate all tables from one or more designated schema(s) to exactly ONE table in downstream?

They should use TablePattern: "*"

But for now, missing field TablePattern is permitted. Would it be troublesome if we forbid it, considering some users may need to modify their old configs?

dm/pkg/router/router_test.go

dm/tests/all_mode/run.sh

lance6716 · 2022-02-14T03:03:54Z

ping @buchuitoudegou

buchuitoudegou · 2022-02-14T09:01:37Z

/run-all-tests

buchuitoudegou · 2022-02-14T09:27:42Z

/run-verify

lance6716 · 2022-02-16T02:18:22Z

/merge

ti-chi-bot · 2022-02-16T02:18:25Z

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

Commit hash: 85dd8ee

close pingcap#4256

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 17, 2022

buchuitoudegou linked an issue Jan 17, 2022 that may be closed by this pull request

dm table-routing need support regular expressions #4256

Closed

buchuitoudegou force-pushed the master branch 2 times, most recently from a2b8e8e to 0040bdc Compare January 18, 2022 02:37

ti-chi-bot removed the do-not-merge/needs-linked-issue label Jan 18, 2022

buchuitoudegou changed the title ~~WIP: new router implementation compatible with regular expression~~ WIP: new router compatible with regular expression Jan 18, 2022

D3Hunter added the area/dm Issues or PRs related to DM. label Jan 18, 2022

buchuitoudegou added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 18, 2022

ti-chi-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 Jan 18, 2022

buchuitoudegou changed the title ~~WIP: new router compatible with regular expression~~ dm: new router compatible with regular expression Jan 18, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2022

buchuitoudegou added 2 commits January 19, 2022 11:18

feat: implement new router compatible with regexpr

1fda1ea

feat: integration test of regexpr router

e293cb7

lance6716 reviewed Jan 19, 2022

View reviewed changes

dm/regexprrouter/router.go Outdated Show resolved Hide resolved

dm/regexprrouter/router.go Outdated Show resolved Hide resolved

dm/regexprrouter/router.go Outdated Show resolved Hide resolved

dm/regexprrouter/router.go Outdated Show resolved Hide resolved

buchuitoudegou force-pushed the master branch from 2dcd595 to 3244936 Compare January 19, 2022 10:37

buchuitoudegou added 6 commits January 19, 2022 18:50

fix: linting error

4397fd8

fix: erroneous copyright

6104c9d

feat: add duplicate matching error

644b62f

feat: using terror and simplify AllRule

d894e80

fix: error msg

3d1570d

Merge remote-tracking branch 'pingcap-origin/master' into dummy

0dcbbcc

Merge remote-tracking branch 'pingcap-origin/master'

9989747

Ehco1996 reviewed Jan 24, 2022

View reviewed changes

buchuitoudegou added 2 commits January 24, 2022 12:08

fix: cleanup data after killing processes

8b0ebbb

Merge remote-tracking branch 'pingcap-origin/master'

6080a27

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2022

GMHDBJD approved these changes Feb 7, 2022

View reviewed changes

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

lance6716 reviewed Feb 10, 2022

View reviewed changes

buchuitoudegou added 2 commits February 14, 2022 15:32

Merge branch 'master' of https://github.com/pingcap/tiflow

08f55c6

fix: pr comment

795beaa

buchuitoudegou force-pushed the master branch from be529fc to 795beaa Compare February 14, 2022 08:59

ti-chi-bot added the do-not-merge/needs-triage-completed label Feb 14, 2022

Merge branch 'master' of https://github.com/pingcap/tiflow

85dd8ee

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2022

lance6716 approved these changes Feb 16, 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 Feb 16, 2022

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

Merge branch 'master' into master

663eabc

ti-chi-bot removed the do-not-merge/needs-triage-completed label Feb 16, 2022

ti-chi-bot merged commit 536d8ae into pingcap:master Feb 16, 2022

zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Feb 16, 2022

dm: new router compatible with regular expression (pingcap#4358)

2045a3a

close pingcap#4256

buchuitoudegou mentioned this pull request Feb 25, 2022

Incompatible table router in DM and Lightning #4701

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm: new router compatible with regular expression #4358

dm: new router compatible with regular expression #4358

buchuitoudegou commented Jan 17, 2022 •

edited

Loading

ti-chi-bot commented Jan 17, 2022 •

edited

Loading

CLAassistant commented Jan 17, 2022 •

edited

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading

buchuitoudegou commented Jan 21, 2022

buchuitoudegou commented Jan 21, 2022

Ehco1996 Jan 24, 2022

buchuitoudegou Jan 24, 2022

GMHDBJD left a comment

lance6716 left a comment

lance6716 Feb 10, 2022

lichunzhu Feb 10, 2022

buchuitoudegou Feb 10, 2022

GMHDBJD Feb 10, 2022

buchuitoudegou Feb 10, 2022

lance6716 commented Feb 14, 2022

buchuitoudegou commented Feb 14, 2022

buchuitoudegou commented Feb 14, 2022

lance6716 commented Feb 16, 2022

ti-chi-bot commented Feb 16, 2022

dm: new router compatible with regular expression #4358

dm: new router compatible with regular expression #4358

Conversation

buchuitoudegou commented Jan 17, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 17, 2022 • edited Loading

CLAassistant commented Jan 17, 2022 • edited Loading

codecov-commenter commented Jan 18, 2022 • edited Loading

Codecov Report

buchuitoudegou commented Jan 21, 2022

buchuitoudegou commented Jan 21, 2022

Ehco1996 Jan 24, 2022

Choose a reason for hiding this comment

buchuitoudegou Jan 24, 2022

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Feb 10, 2022

Choose a reason for hiding this comment

lichunzhu Feb 10, 2022

Choose a reason for hiding this comment

buchuitoudegou Feb 10, 2022

Choose a reason for hiding this comment

GMHDBJD Feb 10, 2022

Choose a reason for hiding this comment

buchuitoudegou Feb 10, 2022

Choose a reason for hiding this comment

lance6716 commented Feb 14, 2022

buchuitoudegou commented Feb 14, 2022

buchuitoudegou commented Feb 14, 2022

lance6716 commented Feb 16, 2022

ti-chi-bot commented Feb 16, 2022

buchuitoudegou commented Jan 17, 2022 •

edited

Loading

ti-chi-bot commented Jan 17, 2022 •

edited

Loading

CLAassistant commented Jan 17, 2022 •

edited

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading