Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

Add zookeeper service #25

Closed
wants to merge 2 commits into from
Closed

Add zookeeper service #25

wants to merge 2 commits into from

Conversation

virtualdxs
Copy link

Per #20 add zookeeper service and set up a logs symlink since zookeeper seems to be ignoring the log4j.properties it's pointed at (I know little about this; I just know I was able to get Kafka running with my changes to this repo).

@id
Copy link
Owner

id commented Dec 16, 2019

I'm sorry but I cannot accept this PR.
It will break many installations where zookeeper does not run on the same host as kafka. When kafka cluster grows beyond 3-5 nodes, having zookeepers running on the same machines becomes a headache.

@virtualdxs
Copy link
Author

It shouldn't break those, as I used Wants instead of Requires. I can remove that and just have After so there's no dependency; would that resolve your concerns?

@id
Copy link
Owner

id commented Dec 16, 2019

No, not really.
I still don't understand the reasoning behind this PR. If you need to install zookeeper, why can't you use separate package for this? https://github.com/id/zookeeper-el7-rpm

@id
Copy link
Owner

id commented Dec 16, 2019

It's much easier to remove zookeeper scripts from this package and leave only zookeeper jar.

@virtualdxs
Copy link
Author

virtualdxs commented Dec 16, 2019

That you'd have to ask the people who included Zookeeper in the Kafka tarball. Feel free to close the PR if you feel that the separate package is preferable.

@id
Copy link
Owner

id commented Dec 16, 2019

Zookeeper library in kafka tarball is a necessity, just as any other third party library there.
But zookeeper shell scripts are not needed - I absolutely agree. The reason they are included in kafka tarball (my gueess) is that it makes it so much easier to quickly spin up kafka node on a local machine.
In production environments, however, an operator usually wants more granular level of control what package is installed where.

@id id closed this Dec 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants