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

Sync kafka settings #179

Merged
merged 2 commits into from
Mar 24, 2021
Merged

Sync kafka settings #179

merged 2 commits into from
Mar 24, 2021

Conversation

hackaugusto
Copy link
Contributor

  • update kafka/zookeeper container versions
  • change from cp-server to cp-kafka which is the community edition
  • made sure the docker-compose and the pytest fixtures use the same settings

Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

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

Version sync comments and version usage needs a bit clarification

KAFKA_CURRENT_VERSION = "2.4"
BASEDIR = "kafka_2.12-2.4.1"
BASEDIR = "kafka_2.13-2.7.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

The line above has KAFKA_CURRENT_VERSION = "2.4", but this PR switches to Kafka 2.7. I see that KAFKA_CURRENT_VERSION is used only for protocol version related settings, but reflecting to comment line before is confusing. What software versions and protocol versions should be defined and where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why KAFKA_CURRENT_VERSION is necessary, I just synchronized the files. Unfortunately setting it to 2.7.0 crashes Kafka

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with updating the comments here (and possibly in Makefile) about what versions need to be in sync. Also what about renaming KAFKA_CURRENT_VERSION to something like KAFKA_PROTOCOL_VERSION?

Copy link
Contributor Author

@hackaugusto hackaugusto Mar 24, 2021

Choose a reason for hiding this comment

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

I must have used an outdated version of the container image. I did check the documentation and tried it again, and the protocol version set as the current version works. Edit: I updated the value

@hackaugusto hackaugusto force-pushed the hacka-sync-compose-and-conf-test branch from aa1087f to cbcbd63 Compare March 24, 2021 15:07
@hackaugusto hackaugusto requested a review from tvainika March 24, 2021 15:30
@tvainika tvainika merged commit 2ef5201 into master Mar 24, 2021
@tvainika tvainika deleted the hacka-sync-compose-and-conf-test branch March 24, 2021 15:47
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