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.

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

kafka: support for multi-topic #4188

Closed
wants to merge 1 commit into from
Closed

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Dec 31, 2021

What problem does this PR solve?

This adds support for writing to multiple kafka topics

Issue Number: ref #1147

What is changed and how it works?

By default the static topic name specified in the URL is used. But tables are registered in the registry by {schema}_{table}.

In #3936 support is added to set the name in the registry to match the topic name. The benefits of this are:

  • Allows multiple unrelated CDC instances with similarly named tables to write to the same kafka broker without conflicts by manually specifying unique topic names.
  • Having the topic and registry names matching, so KSQL etc can use them.
  • With a changefeed per table this works for small-ish setups.

This PR does set the topic name dynamically to {topic_from_url}_{schema}_{table}. This makes this easier to setup and maintain.

Unsolved issues / TODO:

  • Rebase this on top of avro: Add name strategy option for schema registry #3936 once it has been merged and add a naming strategy that matches {topic_from_url}_{schema}_{table} as the current {schema}_{table} naming doesn't match. The prefix is needed in case multiple unrelated schema/table combinations are used with unrelated CDC instances but the same kafka broker.
  • Add a config option for this as we can't enable this by default as that would break backwards compatible behavior
  • Fix the hardcoded NumPartitions: 3 and ReplicationFactor: 1
  • Maybe not create the old static topic name when in multi topic mode
  • Add tests etc.
  • Make sure we set MaxMessageBytes etc. for the topics. Maybe re-unify this with validateMaxMessageBytesAndCreateTopic

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update key monitor metrics in both TiCDC document and official document

Release note

Support for writing to a topic-per-table in Kafka was added

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 31, 2021
@overvenus overvenus added the component/sink Sink component. label Jan 17, 2022
@ti-chi-bot
Copy link
Member

@dveeden: PR needs rebase.

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2022
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.

We are currently implementing it according to the RFC. PRs can be found at #4423.
So I will close this PR. thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/sink Sink component. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants