Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sink(ticdc): Add a topic expression when dispatching messages (Part I) #4540

Merged
merged 12 commits into from
Feb 23, 2022
Merged

sink(ticdc): Add a topic expression when dispatching messages (Part I) #4540

merged 12 commits into from
Feb 23, 2022

Conversation

zhaoxinyu
Copy link
Contributor

@zhaoxinyu zhaoxinyu commented Feb 9, 2022 •
edited by Rustin170506
Loading

What problem does this PR solve?

Issue Number: ref #4423

What is changed and how it works?

Add a topic expression when sinking messages to kafka. For example,

...
[sink]
dispatchers = [
    {matcher = ['s1.t1', 's2.t2'], topic = "{schema}_{table}"}， 
    {matcher = ['s3.*'], topic = "{schema}abc"}
]
...

By using the configuration above, the dispatching rules will be as follows:

  • The data change events in table s1.t1 will be sent to topic "s1_t1".
  • The data change events in table s2.t2 will be sent to topic "s2_t2".
  • The data change events for all tables in s2 will be sent to topic s3abc.

Check List

Tests

  • Unit test

Release note 

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Feb 9, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hi-rustin
  • overvenus

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.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Feb 9, 2022
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 9, 2022
overvenus
overvenus reviewed Feb 9, 2022
View reviewed changes
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression.go Outdated

// When doing conversion, the special characters other than [A-Za-z0-9\._\-] in schema/table
// will be substituted for underscore '_'
func Parse(topicExpr, schema, table string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

How frequent is it invoked?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to call every row change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is called for every RowChangedEvent. I have added a benchmark in the latest commit. From the benchmark, we can see the regex replacement overhead is negligible.

liuzix
liuzix reviewed Feb 9, 2022
View reviewed changes
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member

/run-kafka-integration-test

@3AceShowHand
Copy link
Contributor

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

Rustin170506
Rustin170506 reviewed Feb 10, 2022
View reviewed changes
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks great! There are a few nits.

cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression.go Outdated Show resolved Hide resolved
cdc/sink/dispatcher/expression_test.go Outdated Show resolved Hide resolved
@ti-chi-bot 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 Feb 16, 2022
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 16, 2022
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 16, 2022
@zhaoxinyu zhaoxinyu changed the title sink(ticdc): Add a topic expression when dispatching messages sink(ticdc): Add a topic expression when dispatching messages (Part I) Feb 22, 2022
Rustin170506
Rustin170506 approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Great

cdc/sink/dispatcher/topic/expression_test.go Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2022
@ti-chi-bot 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 23, 2022
@overvenus
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 6fbff6c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 23, 2022
@zhaoxinyu
Copy link
Contributor Author

/run-integration-tests /tidb=pr/32434

@zhaoxinyu
Copy link
Contributor Author

/run-integration-tests /tidb=pr/32434

@zhaoxinyu
Copy link
Contributor Author

/run-dm-integration-test

@ti-chi-bot ti-chi-bot merged commit b1cde57 into pingcap:master Feb 23, 2022
@zhaoxinyu zhaoxinyu mentioned this pull request Feb 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuzix liuzix liuzix left review comments

@overvenus overvenus overvenus approved these changes

@Rustin170506 Rustin170506 Rustin170506 approved these changes

@3AceShowHand 3AceShowHand Awaiting requested review from 3AceShowHand

Assignees
No one assigned
Labels
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. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zhaoxinyu @ti-chi-bot @Rustin170506 @3AceShowHand @overvenus @liuzix