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

Cos integration for KSQL. #663

Merged
merged 7 commits into from
Jan 26, 2018
Merged

Conversation

hjafarpour
Copy link
Contributor

No description provided.

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Some minor stuffs but LGTM


if [ -n "$_permwarn" ]; then
echo "Notice: If you are planning to use the provided systemd service units for"
echo "Notice: confluent-schema-registry, make sure that read-write permissions"
Copy link
Contributor

Choose a reason for hiding this comment

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

schema-registry -> ksql

[Unit]
Description=Streaming SQL engine for Apache Kafka
Documentation=http://docs.confluent.io/
After=network.target confluent-kafka.target
Copy link
Contributor

Choose a reason for hiding this comment

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

add confluent-schema-registry.target as well and any other deps you have
This is not forcing them to start but defines the startup order if those services are enabled locally.

User=cp-ksql
Group=confluent
Environment="LOG_DIR=/var/log/confluent/ksql"
ExecStart=/usr/bin/kafka-ksql-start /etc/kafka-ksql/kafka-ksql.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

I think /etc/kafka-ksql needs to match PACKAGE_TITLE, so /etc/ksql


if [ -n "$_permwarn" ]; then
echo "Notice: If you are planning to use the provided systemd service units for"
echo "Notice: confluent-schema-registry, make sure that read-write permissions"
Copy link
Contributor

Choose a reason for hiding this comment

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

schema-registry -> ksql

%{__mkdir_p} %{buildroot}
%{__cp} -R * %{buildroot}
# The spec file gets included, get rid of it
%{__rm} %{buildroot}/confluent-kafka-rest.spec
Copy link
Contributor

Choose a reason for hiding this comment

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

ksql

%doc
/usr/share/doc/kafka-ksql

%config(noreplace) /etc/kafka-ksql/*
Copy link
Contributor

Choose a reason for hiding this comment

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

match PACKAGE_TITLE

[ "%{buildroot}" != "/" ] && %{__rm} -rf %{buildroot}

%changelog
* Fri Jan 2 2015 Confluent Packaging <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2018 something

@hjafarpour
Copy link
Contributor Author

@edenhill applied your feedback. Please let me know if you find any other issues.

find bin/ -type f | grep -v README[.]rpm | xargs -I XXX $(INSTALL_X) -o root -g root XXX $(DESTDIR)$(PREFIX)/XXX ;\
find share/ -type f | grep -v README[.]rpm | xargs -I XXX $(INSTALL) -o root -g root XXX $(DESTDIR)$(PREFIX)/XXX ; \
pushd etc/ksql/ ; \
find . -type f | grep -v README[.]rpm | xargs -I XXX $(INSTALL) -o root -g root XXX $(DESTDIR)$(SYSCONFDIR)/XXX
Copy link
Contributor

@edenhill edenhill Jan 26, 2018

Choose a reason for hiding this comment

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

hint:
To (manually) verify final package contents, do:

for archive:
tar tvzf output/..ksql..tar.gz

rpm:
rpm -qpl output/..ksql..rpm

deb:
dpkg -c output/..ksql..all.deb

@edenhill
Copy link
Contributor

Don't forget to add the systemd .service file

@edenhill
Copy link
Contributor

It still seems to be missing the confluent-ksql.service file

@hjafarpour
Copy link
Contributor Author

@edenhill yes, just added the missing files.

User=cp-ksql
Group=confluent
Environment="LOG_DIR=/var/log/confluent/ksql"
ExecStart=/usr/bin/ksql-start /etc/ksql/ksql.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

seems the filename is supposed to be ksqlserver.properties?

Type=simple
User=cp-ksql
Group=confluent
Environment="LOG_DIR=/var/log/confluent/ksql"
Copy link
Contributor

Choose a reason for hiding this comment

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

ksql-run-class (or wahtever it is called) needs to be updated to set the ksql.log.dir property based on LOG_DIR, and then change the config/log4j...properties to use ksql.log.dir rather than the hardcoded /tmp/ksql..

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Looks good but the run-class file must be updated to set ksql.log.dir based on LOG_DIR.
See this for an example:
https://github.com/confluentinc/kafka-rest/blob/master/bin/kafka-rest-run-class#L42

@@ -8,17 +8,11 @@ set -ue
#cd -P deals with symlink from /bin to /usr/bin
base_dir=$( cd -P "$( dirname "${BASH_SOURCE[0]}" )/.." && pwd )

# Log directory to use
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should keep this, for providing a defualt log dir (/tmp/..) when LOG_DIR env has not been set.
This also makes sure ksql.log.dir is always set, otherwise the log4j properties will be ran out of the root / dir which will not work.

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

LGTM

@hjafarpour hjafarpour merged commit 03c33cd into confluentinc:master Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants