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

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

Merged
ti-chi-bot merged 19 commits into
pingcap:masterfrom
wentaojin:master
Sep 23, 2021
Merged

onlineddl: support regular expressions rules for PT/GH-OST #2139
ti-chi-bot merged 19 commits into
pingcap:masterfrom
wentaojin:master

Conversation

@wentaojin

@wentaojin wentaojin commented Sep 15, 2021

Copy link
Copy Markdown
Contributor

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

ti-chi-bot commented Sep 15, 2021

Copy link
Copy Markdown
Member

[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.

Details

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
Comment thread syncer/online-ddl-tools/online_ddl.go
Comment thread dm/config/subtask.go Outdated
Comment thread dm/config/subtask.go Outdated
Comment thread dm/config/subtask.go Outdated
Comment thread dm/config/subtask.go Outdated
Comment thread dm/config/subtask.go Outdated
Comment thread syncer/syncer.go Outdated
@Ehco1996

Ehco1996 commented Sep 16, 2021

Copy link
Copy Markdown
Contributor

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 thread dm/config/subtask.go Outdated
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
Copy Markdown
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"`

Comment thread dm/config/task.go Outdated

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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ditto

Comment thread pkg/terror/error_list.go Outdated
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
Copy Markdown
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.

Comment thread dm/config/subtask.go Outdated

@Ehco1996 Ehco1996 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rest LGTM

Comment thread dm/config/subtask.go
adjustedRules := make([]string, 0, len(rules))
for _, r := range rules {
if !strings.HasPrefix(r, "^") {
r = "^" + r

Copy link
Copy Markdown
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

@lichunzhu lichunzhu Sep 22, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread syncer/syncer_test.go Outdated
@lichunzhu

Copy link
Copy Markdown
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
Copy Markdown
Contributor

/run-unit-tests

@lichunzhu

Copy link
Copy Markdown
Contributor

/merge

@ti-chi-bot

Copy link
Copy Markdown
Member

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

DetailsCommit hash: 5850fb4

@ti-chi-bot

Copy link
Copy Markdown
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