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

Support kafka 3.0 topics #337

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Support kafka 3.0 topics #337

merged 1 commit into from
Apr 25, 2022

Conversation

der-eismann
Copy link
Contributor

Pull Request (PR) description

In kafka 3.0 the --zookeeper option was removed for topics. It was deprecated in v2.2.0 (March 2019) and --bootstrap-server has been available since then. I'm unsure if we should replace zookeeper with bootstrap-server completely or allow both options, but in that case I don't know how to require at least one of both arguments. Open for discussion here.

This Pull Request (PR) fixes the following issues

Fixes #302

Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Hello and thank you for the PR!

Take a look at the inline comments.

Regarding dropping the zookeeper option:
Given that ZooKeeper and bootstrap server are two different subsystems I suggest that we keep both of the options for now. Also, some versions of Kafka that can use zookeeper option are not yet EoL (e.g 2.4.x).

To have one of the two options defined i think it can be achieved with:

if !$zookeeper && !$bootstrap_server {
  fail('Either zookeeper or bootstrap_server parameter must be defined!')
}

manifests/topic.pp Outdated Show resolved Hide resolved
manifests/topic.pp Outdated Show resolved Hide resolved
manifests/topic.pp Outdated Show resolved Hide resolved
@root-expert root-expert added the enhancement New feature or request label Mar 14, 2022
@der-eismann
Copy link
Contributor Author

Thanks for the quick replies! I added all the changes, looks better now 🙂
Regarding the failing test I think #334 needs to be merged because there are no puppet 7 packages for Debian Jessie (8).

manifests/topic.pp Outdated Show resolved Hide resolved
manifests/topic.pp Show resolved Hide resolved
manifests/topic.pp Show resolved Hide resolved
Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Please squash your commits before merging 😄

@der-eismann
Copy link
Contributor Author

Please squash your commits before merging smile

Done!

@root-expert root-expert merged commit 37f50c8 into voxpupuli:master Apr 25, 2022
@der-eismann der-eismann deleted the support-kafka-3 branch April 25, 2022 15:03
@der-eismann
Copy link
Contributor Author

Thanks for merging and your support!

@evgenkisel evgenkisel mentioned this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Topic] Parameter --zookeeper
3 participants