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

DBZ-8069 Add override.data.change.topic.prefix property to TableTopicNamingStrategy #203

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

twthorn
Copy link
Contributor

@twthorn twthorn commented Jul 15, 2024

See https://issues.redhat.com/browse/DBZ-8069 for more context on why this needed

See also this Zulip chat on why we went this route https://debezium.zulipchat.com/#narrow/stream/302529-community-general/topic/Separate.20logs.2Fmetrics.20for.20identical.20topic.2Eprefix.20connectors

Expand our current custom vitess topic naming strategy to support this additional config.

@twthorn
Copy link
Contributor Author

twthorn commented Jul 15, 2024

@jpechane this one is also ready for review. thanks in advance!

Copy link
Member

@Naros Naros left a comment

Choose a reason for hiding this comment

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

This LGTM, just left one specific suggestion for completeness.

@@ -29,7 +34,13 @@ public static TableTopicNamingStrategy create(CommonConnectorConfig config) {

@Override
public String dataChangeTopic(TableId id) {
String topicName = mkString(Collect.arrayListOf(prefix, id.table()), delimiter);
String topicName;
if (overrideDataChangeTopicPrefix != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (overrideDataChangeTopicPrefix != null) {
if (!Strings.isNullOrBlank(overrideDataChangeTopicPrefix)) {

@twthorn twthorn requested a review from Naros July 18, 2024 16:41
@Naros
Copy link
Member

Naros commented Jul 18, 2024

Thanks @twthorn, would you please open an accompanying PR in the main repository to document this new configuration option for the table naming strategy for Vitess?

@twthorn
Copy link
Contributor Author

twthorn commented Jul 18, 2024

@Naros Yes, of course, here's the PR to update docs debezium/debezium#5698

Copy link
Member

@Naros Naros left a comment

Choose a reason for hiding this comment

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

Thanks @twthorn LGTM now.

@Naros Naros merged commit 9288a28 into debezium:main Jul 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants