-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Kafka 3.1.0 to CI matrix, migrate to bitnami kafka image #2124
Conversation
We noticed that this PR either modifies or introduces code that pipes the output of |
@bai I’m +1 for moving away from proprietary images so we can get closer to main releases of kafka. However, should we just build our own images rather than rely on bitnami ones? |
I'll take a look at that; want to see if bitnami even works and improves the compatibility. To be honest I'm a bit "lazy" to maintain our own Kafka docker images since IIRC it's not trivial to build them - albeit we have to do it once. |
Oh wow, didn't think it's going to work just like that. @dnwe could you please take a look at this PR once again? 🙏🏼 (Obviously I'll squash and rename commits, but wanted to see if it looks good to you.) |
@@ -1,7 +1,7 @@ | |||
version: '3.7' | |||
services: | |||
zookeeper-1: | |||
image: 'confluentinc/cp-zookeeper:${CONFLUENT_PLATFORM_VERSION:-7.0.1}' | |||
image: 'confluentinc/cp-zookeeper:7.0.1' |
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.
I don't expect Zookeeper version requirement to change as often as Kafka versions change, so oped to hardcode Zookeeper version instead of adding an extra configuration parameter.
We might explore moving to non-proprietary zookeeper docker image in future.
@bai good work — I wonder if it's worth keeping 2.8.1 around in the CI matrix for a bit longer as well? Whilst I know our official support statement is N-1, I think as 3.1.0 has only just released we probably want a bit of coverage on the 2.x series Aside: I wonder if we should split up the matrix so that we run the non-functional unittests with Go 1.16.x and 1.17.x and then run the functional tests against the Kafka versions matrix with just 1.17.x |
@dnwe — thanks for feedback. I've updated the build matrix to reflect it, I think it's a great suggestion. Let me know what you think. |
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.
Looks great!
Migrate from cp-kafka proprietary kafka docker image to bitnami one.
As you could see in the diff, we've had quite some hackery around handling the fact that Confluent Platform versioning scheme is very off with Kafka one.
Not only that, Confluent Platform lags behind quite noticeably — we add workarounds to pretend we're really testing against the latest Kafka version, while we're not.
To mitigate these annoyances, in this PR I'm migrating to a more actively maintained Bitnami Kafka docker image that seems to be pretty up to date with mainstream Kafka releases. Thanks Bitnami folks!