Support Kraft post Confluent Platform 7.4.0 by danielpetisme · Pull Request #7014 · testcontainers/testcontainers-java

danielpetisme · 2023-05-07T16:15:50Z

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).

eddumelendez · 2023-05-07T17:26:16Z

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

eddumelendez · 2023-05-07T19:09:54Z

modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java

    protected void configureKraft() {
+        //CP 7.4.0
+        withEnv("KAFKA_CONTROLLER_QUORUM_MODE", "kraft");
+        withEnv("CLUSTER_ID", getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId));


I think we can use it as

Suggested change

withEnv("CLUSTER_ID", getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId));

getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId);

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" ); } ) );

yes, please

eddumelendez · 2023-05-08T12:58:20Z

@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

stejani-cflt · 2023-05-09T01:00:50Z

modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java

        copyFileToContainer(Transferable.of(command, 0777), STARTER_SCRIPT);
    }

    protected String commandKraft() {


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 ()

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

agree, the module is already complex enough 😅 Let's rollback that change, please

…into fix/support-cp-7.4

danielpetisme · 2023-05-09T14:44:56Z

@eddumelendez I'm done with everything I wanted to update.
Ready for a full review

eddumelendez · 2023-05-09T15:48:30Z

docs/modules/kafka.md

+### <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-->


any reason to move it? I guess there is preferences but I think it doesn't hurt 😄

Yep, definitively Kraft is the preferred option from now on.

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?

I think that's up to the user. That's why I said it doesn't hurt.

eddumelendez · 2023-05-09T23:59:16Z

Thanks for your contribution, @danielpetisme ! This is now in main branch and it will part of the next release.

danielpetisme · 2023-05-10T12:47:58Z

Thanks @eddumelendez.

Do you know when the next release would occur?
I created/closed an issue to make the Kraft+CP.4 scenario explicit and help users to find a clear status on this

eddumelendez · 2023-05-10T14:38:11Z

I'm planning to do the release as soon as GitHub is stable.

danielpetisme added 2 commits May 7, 2023 18:07

Fixing Kraft support for CP 7.4.0

0e5fc6f

Code formatting

0288f10

github-actions bot added the modules/kafka label May 7, 2023

eddumelendez marked this pull request as ready for review May 7, 2023 17:46

eddumelendez requested a review from a team May 7, 2023 17:46

eddumelendez added the type/enhancement label May 7, 2023

eddumelendez reviewed May 7, 2023

View reviewed changes

stejani-cflt reviewed May 9, 2023

View reviewed changes

modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java Outdated Show resolved Hide resolved

stejani-cflt reviewed May 9, 2023

View reviewed changes

modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java Outdated Show resolved Hide resolved

stejani-cflt reviewed May 9, 2023

View reviewed changes

danielpetisme added 5 commits May 9, 2023 16:27

Fixing reviews

d1ca523

Fixing reviews

4cbc82d

Merge branch 'main' of github.com:testcontainers/testcontainers-java …

66ff671

…into fix/support-cp-7.4

Updating documentation

14a3148

Updating documentation

29731d7

eddumelendez added this to the next milestone May 9, 2023

github-actions bot added the type/docs label May 9, 2023

eddumelendez reviewed May 9, 2023

View reviewed changes

Polish

ab8c1e3

eddumelendez approved these changes May 9, 2023

View reviewed changes

eddumelendez merged commit c64aab9 into testcontainers:main May 9, 2023

This was referenced May 10, 2023

Remove superficial Kafka optimization for the sake of clarity #7030

Closed

[Bug]: Kafka Kraft mode is not working with Confluent Platform 7.4.0 or above images #7031

Closed

danielpetisme deleted the fix/support-cp-7.4 branch May 10, 2023 12:54

	withEnv("CLUSTER_ID", getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId));
	getEnvMap().computeIfAbsent("CLUSTER_ID", key -> clusterId);

Uh oh!

Conversation

danielpetisme commented May 7, 2023

Uh oh!

eddumelendez commented May 7, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddumelendez commented May 8, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielpetisme commented May 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddumelendez commented May 9, 2023

Uh oh!

danielpetisme commented May 10, 2023

Uh oh!

eddumelendez commented May 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants