KAFKA-5295: Allow source connectors to specify topic-specific settings for new topics (KIP-158)#8722
Conversation
|
Rebased to resolve conflicts |
There was a problem hiding this comment.
These had to be moved out of this class, or else tests for StandaloneHerder would break. They are now reused here as well as TopicCreationConfig
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
Nice work, @kkonstantine! I completed an initial pass, and overall this looks good. Detailed questions and comments in line comments below.
|
Thanks for your comments @rhauch ! I'll ping you here when it's ready for another pass. Thanks! |
kkonstantine
left a comment
kkonstantine
left a comment
There was a problem hiding this comment.
Thanks again for the first round of comments @rhauch
I think I've addressed them all with a fix or a comment.
|
Results of the latest completed build: |
|
Rebased just to resolve conflicts with #2604 |
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
Thanks for the update, @kkonstantine. Took a second pass -- things look good, and I'm finding smaller things to nit pick.
|
Rebased to resolve minor conflicts from #8118 |
Co-authored-by: Randall Hauch <rhauch@gmail.com>
kkonstantine
left a comment
kkonstantine
left a comment
There was a problem hiding this comment.
Thanks for the follow up @rhauch !
I've pushed the new commits to address your comments and I've rebased (and fixed a previous rebase) to resolve conflicts.
The only suggestion I haven't followed yet is the one around the ordering of static methods. I think I've followed our style here for the most part, wdyt?
Besides that, I'd like to revisit once more the new unit tests in one more commit, after the refactoring that you suggested.
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
Thanks, @kkonstantine. Even closer now, though I have a few comments/suggestions.
Co-authored-by: Randall Hauch <rhauch@gmail.com>
Co-authored-by: Randall Hauch <rhauch@gmail.com>
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
One comment to fix my screwed-up suggestion.
| // If the user has added regex of include or exclude patterns in the default group, | ||
| // they should be ignored. |
kkonstantine
left a comment
kkonstantine
left a comment
There was a problem hiding this comment.
Thanks @rhauch !
I think I've addressed your latest comments.
Lmk how it looks. Hopefully we'll get a couple green builds too.
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
Great work, @kkonstantine! It's going to be really nice to get this in, so thanks for picking this up.
LGTM, pending a green build.
|
One test broke after the change in the config validation exception. Should be fixed now. |
|
jdk8: success Given these results I'll go ahead and merge this PR. |
2 participants
Kafka Connect workers have been able to create Connect's internal topics using the new admin client for some time now (see KAFKA-4667). However, tasks of source connectors are still relying upon the broker to auto-create topics with default config settings if they don't exist, or expect these topics to exist before the connector is deployed, if their configuration needs to be specialized.
With the implementation of KIP-158 here, if
topic.creation.enable=true, Kafka Connect will supply the source tasks of connectors that are configured to create topics with an admin client that will allow them to create new topics on-the-fly before writing the first source records to a new topic. Additionally, each source connector has the opportunity to customize the topic-specific settings of these new topics by defining groups of topic configurations.This feature is tested here via unit tests (old tests that have been adjusted and new ones) as well as integration tests.
Committer Checklist (excluded from commit message)