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

Jump to bottom

KAFKA-13397: Honor 'replication.policy.separator' configuration when creating MirrorMaker2 internal topics #11431

Merged
merged 2 commits into from Nov 17, 2021
Merged

KAFKA-13397: Honor 'replication.policy.separator' configuration when creating MirrorMaker2 internal topics #11431

merged 2 commits into from Nov 17, 2021

Conversation

dongjinleekr
Copy link
Contributor

I found this issue while working on KAFKA-13365. Since KAFKA-10777 is scheduled to be shipped with 3.1.0, this issue would be better to be fixed.

cc/ @ryannedolan @mimaison @OmniaGM

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@dongjinleekr
1. Throw ConfigException when a user tries to use a single dash as a …
7ca2631 
…replication policy separator. 2. Honor replication.policy.separator when creating MM2 internal offsets, status, config topics.
mimaison
mimaison reviewed Oct 25, 2021
View reviewed changes
Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks like you uncovered some real issues with driver mode. I left a few comments.

connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/MirrorClientConfig.java Outdated Show resolved Hide resolved
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java Outdated Show resolved Hide resolved
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java Outdated Show resolved Hide resolved
OmniaGM
OmniaGM reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@OmniaGM OmniaGM left a comment

Choose a reason for hiding this comment

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

Thanks for your pr and for spotting this. I left a few comments.

connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/MirrorClientConfig.java Outdated Show resolved Hide resolved
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java Outdated Show resolved Hide resolved
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java Outdated Show resolved Hide resolved
@dongjinleekr
Copy link
Contributor Author

So, We can now organize the issues like the following:

  1. Preventing the malfunctioning (i.e., replication of MM2 internal topics)
  2. Keeping consistency (as far as possible.)
  3. Maintaining backward compatibility (we MUST.)

My approach focuses on standalone mode with 1 and 2, and @OmniaGM 's approach (i.e., fixing isInternalTopic method) focuses on connect mode with 1 and 3. If we agree on this, we are now on the same page. Isn't it?

mimaison
mimaison reviewed Nov 3, 2021
View reviewed changes
Copy link
Member

@mimaison mimaison left a comment •
edited

Choose a reason for hiding this comment

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

So in the end I don't think we want Kafka Connect internal topics to reuse the configured separator of MirrorMaker topics. Or if it's something you want to do then we'll need a KIP.

See my comment in #11431 (comment)

@dongjinleekr
1. Revert to the original implementation + update ReplicationPolicy#i…
1e7ffa6 
…sInternalTopic. 2. Add ReplicationPolicyTest.
@dongjinleekr
Copy link
Contributor Author

@mimaison Here is the fix.

I added ReplicationPolicyTest to validate the implementation. It seems like we need to test the other methods but, let's focus on DefaultReplicationPolicy#isInternalTopic only in this PR. (I will open a separate PR for it later.)

@OmniaGM Thanks for suggesting the right alternative. 🙇

@mimaison
Copy link
Member

@dongjinleekr I think it's probably something we want in 3.1 otherwise KIP-690 may break some users, WDYT?
cc @dajac

@dongjinleekr
Copy link
Contributor Author

@mimaison @dajac Whatever it is, I will follow your (plural) decision; If you think we need a hotfix KIP, I can do it by this Friday.

mimaison
mimaison approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@mimaison mimaison merged commit ef94af1 into apache:trunk Nov 17, 2021
@mimaison
Copy link
Member

@dajac Can we apply with on 3.1? Without this fix, when using MirrorMaker in dedicated mode and overriding the replication policy separator, Connect internal topics get mirrored.

@mimaison
Copy link
Member

@dongjinleekr I'll rename KAFKA-13397 to match the fix. Then if you want to propose a further KIP/PR to actually honor the separator we can use a new JIRA.

mimaison pushed a commit that referenced this pull request Nov 24, 2021
@dongjinleekr @mimaison
KAFKA-13397: MirrorMaker should not mirror topics ending with `.inter…
417630d 
…nal` (#11431)

When running in dedicated mode, Connect runtimes are configured to use the `.internal` suffix for their topics. 

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Omnia G H Ibrahim <o.g.h.ibrahim@gmail.com>
@mimaison
Copy link
Member

Backported to 3.1: 417630d

hgeraldino pushed a commit to hgeraldino/kafka that referenced this pull request Nov 29, 2021
@dongjinleekr @hgeraldino
KAFKA-13397: MirrorMaker should not mirror topics ending with `.inter…
08c4f69 
…nal` (apache#11431)

When running in dedicated mode, Connect runtimes are configured to use the `.internal` suffix for their topics. 

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Omnia G H Ibrahim <o.g.h.ibrahim@gmail.com>
xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
@dongjinleekr
KAFKA-13397: MirrorMaker should not mirror topics ending with `.inter…
7bebabd 
…nal` (apache#11431)

When running in dedicated mode, Connect runtimes are configured to use the `.internal` suffix for their topics. 

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Omnia G H Ibrahim <o.g.h.ibrahim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmniaGM OmniaGM OmniaGM left review comments

@mimaison mimaison mimaison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@dongjinleekr @mimaison @OmniaGM