-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix confluent-schema-registry.service dependencies #917
base: master
Are you sure you want to change the base?
Conversation
confluent-kafka.target is not found, shoudl be "service" moreover added dependency upon confluent-zookeper.service
It looks like @roccogerminario hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
The registry does not necessarily require Zookeeper. Through, transitively, it does via Kafka. How does systemd handle transitive service dependency ordering? |
The problem is not just with the schema-registry but for all the other services as well: confluent-kafka-connect.service, confluent-kafka-rest.service, confluent-kafka.service and confluent-ksql.service. |
Hmm. In my testing using the Ansible Playbooks, the services install and start fine. Though, I do understand your point about the "typo" of target vs service |
I installed on "Centos 7" with "yum"... |
That's the OS I tested using a VM cluster https://github.com/cricket007/cp-ansible/blob/addVagrant/vagrant/README.md |
@edenhill can you take a look at this PR when you get a chance? |
I don't believe the zookeeper dependency should be needed, isn't the change to |
This, and all of the other confluent systemd units, still have broken dependencies due to these typos. Confirmed on Debian. |
There is no guarantee that CP services will be deployed onto the same system. Therefore this is a not-desireable change. If customers are doing co-located deployments of CP services and want systemd unit files "aware" then they need to modify the systemd unit files themselves with tools such as ansible or puppet. "out of the box" its assumed that services are deployed are isolated. I propose that this change be dropped. |
I understand that, but this is still a typo. If you do not support colocated deployments, then a better way to do that would be to remove the declared systemd dependencies, not to have broken dependencies. The default configuration files shipped with the Confluent packages for Debian/Ubuntu are definitely set up for colocated services on the same machine. |
@@ -1,7 +1,7 @@ | |||
[Unit] | |||
Description=RESTful Avro schema registry for Apache Kafka | |||
Documentation=http://docs.confluent.io/ | |||
After=network.target confluent-kafka.target | |||
After=network.target confluent-zookeeper.service confluent-kafka.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After=network.target confluent-zookeeper.service confluent-kafka.service |
Then we should be removing the dependencies altogether do you agree? (See my suggested edit) - All of this was set up before I joined, so I can't speak to the reason why it was .target and not .service Also speaking as the release master, I won't be making this change within a released version of CP (So you still need to use puppet/ansible to set your desired state), but would make one for the next major version (7.1 currently), as it represents a behavior change. |
No, I don't agree. The Confluent Platform on-premise installation instructions for Debian and Ubuntu walk through a single-node setup, and those instructions don't quite work due to the typos in the systemd units. If you are doing a multi-node setup and you simply don't have systemd units installed for the |
|
confluent-kafka.target is not found, should be ".service"
moreover added dependency upon confluent-zookeper.service