Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

onlineddl: support regular expressions rules for PT/GH-OST #2139

Merged
merged 19 commits into from
Sep 23, 2021

Conversation

wentaojin
Copy link
Contributor

@wentaojin wentaojin commented Sep 15, 2021

What problem does this PR solve?

Solve the problem of custom PT/GH-OST format

This feature is added because some users may modify the pt/gh-ost code because they are worried about the business mistakenly deleting the table, which may cause the format to be inconsistent with the Dm, which affects synchronization,the feature can solve it

close #2123

What is changed and how it works?

Add DM config,if values is null, dm will keep default , if values is not null , match according to specific value

shadow-table-rule: [] # 支持正则,默认 ^_(.+)_(?:new|gho)$ 匹配多个具体值后加 (.*)
trash-table-rule: [] # 支持正则, 默认 ^_(.+)_(?:ghc|del|old)$ 匹配多个具体值后加 (.*)

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 15, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • glorv

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.

@glorv glorv added this to the v2.0.7 milestone Sep 15, 2021
@Ehco1996 Ehco1996 added needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Sep 16, 2021
dm/config/subtask.go Outdated Show resolved Hide resolved
dm/config/subtask.go Outdated Show resolved Hide resolved
dm/config/subtask.go Outdated Show resolved Hide resolved
dm/config/subtask.go Outdated Show resolved Hide resolved
dm/config/subtask.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
@Ehco1996
Copy link
Contributor

Ehco1996 commented Sep 16, 2021

Please fix the integration test error on CI as well 🤣

This is because you have changed the config of the task and can be fixed by referring to this commit

Note that in your case you should not use diff -i to ignore the inconsistency, but rather set shadow-table-rule and trash-table-rule in task.yaml and check that they are euqal after export

Comment on lines 198 to 199
ShadowTableRule []string `yaml:"shadow-table-rule" toml:"shadow-table-rule" json:"shadow-table-rule"`
TrashTableRule []string `yaml:"trash-table-rule" toml:"trash-table-rule" json:"trash-table-rule"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ShadowTableRule []string `yaml:"shadow-table-rule" toml:"shadow-table-rule" json:"shadow-table-rule"`
TrashTableRule []string `yaml:"trash-table-rule" toml:"trash-table-rule" json:"trash-table-rule"`
ShadowTableRules []string `yaml:"shadow-table-rules" toml:"shadow-table-rules" json:"shadow-table-rules"`
TrashTableRules []string `yaml:"trash-table-rules" toml:"trash-table-rules" json:"trash-table-rules"`

@@ -302,6 +302,10 @@ type TaskConfig struct {
MySQLInstances []*MySQLInstance `yaml:"mysql-instances" toml:"mysql-instances" json:"mysql-instances"`

OnlineDDL bool `yaml:"online-ddl" toml:"online-ddl" json:"online-ddl"`
// pt/gh-ost name rule,support regex
ShadowTableRule []string `yaml:"shadow-table-rule" toml:"shadow-table-rule" json:"shadow-table-rule"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -877,6 +879,10 @@ var (
ErrConfigGenTableRouter = New(codeConfigGenTableRouter, ClassConfig, ScopeInternal, LevelHigh, "generate table router error", "Please check the `routes` config in task configuration file.")
ErrConfigGenColumnMapping = New(codeConfigGenColumnMapping, ClassConfig, ScopeInternal, LevelHigh, "generate column mapping error", "Please check the `column-mappings` config in task configuration file.")
ErrConfigInvalidChunkFileSize = New(codeConfigInvalidChunkFileSize, ClassConfig, ScopeInternal, LevelHigh, "invalid `chunk-filesize` %v", "Please check the `chunk-filesize` config in task configuration file.")
ErrConfigOnlineDDLRegexCompile = New(codeConfigOnlineDDLRegexCompile, ClassConfig, ScopeInternal, LevelHigh,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge this two error code together, the only reason that regexp compile fail is that the syntax is not correct.

dm/config/subtask.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Ehco1996 Ehco1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

adjustedRules := make([]string, 0, len(rules))
for _, r := range rules {
if !strings.HasPrefix(r, "^") {
r = "^" + r
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it would be better to just return the error here rather than adjust re ((add "^" and "$") ) for the user

Copy link
Contributor

@lichunzhu lichunzhu Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this way is simpler for users. They need to check the error message and rewrite task.yaml if we return an error here. Waiting for other reviewers' suggestions...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok if noted in document.

syncer/syncer_test.go Outdated Show resolved Hide resolved
@lichunzhu
Copy link
Contributor

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Sep 22, 2021
@ti-chi-bot ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Sep 22, 2021
@lichunzhu
Copy link
Contributor

/run-unit-tests

@lichunzhu
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 5850fb4

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #2156.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
first-time-contributor needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 size/L status/can-merge status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants