-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support Kraft post Confluent Platform 7.4.0 #7014
Support Kraft post Confluent Platform 7.4.0 #7014
Conversation
Thank you @danielpetisme ! This is something we noticed last week. I'll take a look later 🙂 |
modules/kafka/src/test/java/org/testcontainers/containers/KafkaContainerTest.java
Show resolved
Hide resolved
modules/kafka/src/test/java/org/testcontainers/containers/KafkaContainerTest.java
Show resolved
Hide resolved
@@ -136,6 +144,9 @@ protected void configure() { | |||
} | |||
|
|||
protected void configureKraft() { | |||
//CP 7.4.0 | |||
withEnv("KAFKA_CONTROLLER_QUORUM_MODE", "kraft"); | |||
withEnv("CLUSTER_ID", getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId)); |
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 think we can use it as
withEnv("CLUSTER_ID", getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId)); | |
getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId); |
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 like it works...
Should I change the syntax of the following lines too?
withEnv(
"KAFKA_NODE_ID",
getEnvMap().computeIfAbsent("KAFKA_NODE_ID", key -> getEnvMap().get("KAFKA_BROKER_ID"))
);
and
withEnv(
"KAFKA_CONTROLLER_QUORUM_VOTERS",
getEnvMap()
.computeIfAbsent(
"KAFKA_CONTROLLER_QUORUM_VOTERS",
key -> {
return String.format(
"%s@%s:9094",
getEnvMap().get("KAFKA_NODE_ID"),
getNetwork() != null ? getNetworkAliases().get(0) : "localhost"
);
}
)
);
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.
yes, please
@danielpetisme LMK when you feel comfortable about reviewing this PR. I noticed that somehow I marked as a ready for review |
modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
Outdated
Show resolved
Hide resolved
modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
Outdated
Show resolved
Hide resolved
|
||
// Optimization: skip the checks | ||
command += "echo '' > /etc/confluent/docker/ensure \n"; | ||
// Run the original command | ||
command += "/etc/confluent/docker/run \n"; | ||
copyFileToContainer(Transferable.of(command, 0777), STARTER_SCRIPT); | ||
} | ||
|
||
protected String commandKraft() { |
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.
Is this only called for < CP 7.4? If yes, it may be good to add a defense in depth and check that it is indeed isLessThanCP740 ()
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.
Yes, it supposed to be invoked in Zk mode or when Kraft AND CP <7.4
https://github.com/testcontainers/testcontainers-java/pull/7014/files#diff-7c5a407b71c96d4816697ed454df5cb084987573025af294ffa6c182dbd8879eR199-R209
Would like to add a test?
TBH, I'm questioning the optimization gains of removing the checks /etc/confluent/docker/ensure
contains... For now, I suggested this solution to be fully backward compatible.
@eddumelendez here are the checks the script is performing... IMHO, the gain does not worth the "complexity" of the code... but that's my 2cts...
https://github.com/confluentinc/kafka-images/blob/master/kafka/include/etc/confluent/docker/ensure
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.
agree, the module is already complex enough 😅 Let's rollback that change, please
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.
Done
#7030
@eddumelendez I'm done with everything I wanted to update. |
docs/modules/kafka.md
Outdated
### <a name="zookeeper"></a> Using external Zookeeper | ||
|
||
If for some reason you want to use an externally running Zookeeper, then just pass its location during construction: | ||
<!--codeinclude--> | ||
[External Zookeeper](../../modules/kafka/src/test/java/org/testcontainers/containers/KafkaContainerTest.java) inside_block:withExternalZookeeper | ||
<!--/codeinclude--> |
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.
any reason to move it? I guess there is preferences but I think it doesn't hurt 😄
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.
Yep, definitively Kraft is the preferred option from now on.
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.
Sounds like this change did not get merged. 😢
@eddumelendez as said, Kraft would be the preferred option, would you mind to invert the order and put Kraft first?
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 think that's up to the user. That's why I said it doesn't hurt.
Thanks for your contribution, @danielpetisme ! This is now in |
Thanks @eddumelendez.
|
I'm planning to do the release as soon as GitHub is stable. |
As part of Confluent Platform 7.4.0, Docker images have been updated to simplify Kraft support.
confluentinc/kafka-images#209
This PR aims to take these changes into consideration while still supporting the older Docker images (CP 7.0.x-7.3.x).