Add Kafka module by bsideup · Pull Request #546 · testcontainers/testcontainers-java

bsideup · 2018-01-16T19:38:13Z

No description provided.

bsideup · 2018-01-16T19:39:51Z

core/src/main/java/org/testcontainers/containers/Network.java


        @Override
-        public String getId() {
+        public synchronized String getId() {


Nasty bug discovered during my experiments with Kafka module, concurrent access was giving null for the second invocation

Have you tried making id volatile. This should work and is cheaper under contention that synchronized method

not a performance critical part (being called just a few times), doesn't make sense to over-optimize it :)

Train your good habits! :)

keep it simple 😎

Isn't volatile as simple? Doesn't really matter for me in this case 😁

bsideup · 2018-01-16T19:41:30Z

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

+
+        withEnv("KAFKA_ZOOKEEPER_CONNECT", "localhost:2181");
+        withClasspathResourceMapping("tc-zookeeper.properties", "/zookeeper.properties", BindMode.READ_ONLY);
+        withCommand("sh", "-c", "zookeeper-server-start /zookeeper.properties & /etc/confluent/docker/run");


I've decided to implement all-in-one container simply to speed it up. Starting from Kafka 0.11, Zookeeper is no longer have to be exposed to the clients and becomes a detail of implementation of Kafka, so IMO it's fine

I would personally prefer to have those as separate containers. or at least provide this as an option to configure. this will allow testing scenarios of network partitions between zookeeper and kafka nodes.

bsideup · 2018-01-16T19:42:30Z

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

+        withEnv("KAFKA_BROKER_ID", "1");
+        withEnv("KAFKA_LISTENERS", "PLAINTEXT://0.0.0.0:9092,BROKER://127.0.0.1:9093");
+        withEnv("KAFKA_LISTENER_SECURITY_PROTOCOL_MAP", "BROKER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
+        withEnv("KAFKA_INTER_BROKER_LISTENER_NAME", "BROKER");


This line makes the whole thing work :D Otherwise, Kafka would try to communicate with itself with host's port (yes, even in 1-node scenario (sic!) )

I'm not directly familiar with Kafka or the 7 environment variables that are being set in this method. Is it possible that someone might want to use alternative values for some of these? If so, they could happily subclass and re-set the env vars they want, so that's OK.

However, would it be worth having a comment in the code (similar to your Github comment!) to say which settings are just critical to getting Kafka to work in the Dockerised environment and shouldn't be changed?

bsideup · 2018-01-16T19:45:07Z

core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java

    }

+    public String getSocatContainerImage() {
+        return (String) properties.getOrDefault("socat.container.image", "alpine/socat:latest");


I had to use :latest :(
https://hub.docker.com/r/alpine/socat/tags/

Even tho there is a tag, it seems to be old and didn't work for me. But I assume it's fine to use latest here because socat itself is supposed to be backward compatible and shouldn't break over the time

Or would it be more responsible to maintain our own image? 😕

once we get our name on Docker Hub - sure, for now I would stick with Alpine's

gAmUssA · 2018-01-16T19:58:29Z

Yay111
We need dis!

…iner

kiview · 2018-01-17T13:52:45Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java


+    private final AtomicInteger nextAmbassadorPort = new AtomicInteger(2000);
+    private final Map<String, Map<Integer, Integer>> ambassadorPortMappings = new ConcurrentHashMap<>();
+    private final SocatContainer ambassadorContainer = new SocatContainer();


Shouldn't this changes happen in a different PR? It's a quite big one.

there is little to no activity around DockerComposeContainer, so I would keep it as part of this PR until you have objections :)

I'll try this change on Windows tonight 😉

All good on Docker for Windows 💪

kiview · 2018-01-17T13:55:30Z

core/src/main/java/org/testcontainers/containers/SocatContainer.java

+    protected void configure() {
+        withCommand("-c",
+                targets.entrySet().stream()
+                        .map(entry -> "socat TCP-LISTEN:" + entry.getKey() + ",fork,reuseaddr TCP:" + entry.getValue())


Are we sure that strange hostname values won't break the shell if we concat the String like this?

AFAIK any valid hostname should be valid shell argument

kiview · 2018-01-17T13:56:23Z

core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java

    }

+    public String getSocatContainerImage() {
+        return (String) properties.getOrDefault("socat.container.image", "alpine/socat:latest");


Or would it be more responsible to maintain our own image? 😕

kiview · 2018-01-17T14:01:47Z

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

+        this("4.0.0");
+    }
+
+    public KafkaContainer(String confluencePlatformVersion) {


AFAIK it's confluent, not confluence.

WDYT about allowing non-confluent images? We are i.e. using https://hub.docker.com/r/ches/kafka/ for our acceptance-tests. This image is missing KAFKA_INTER_BROKER_LISTENER_NAME, but it should be trivial to update the image accordingly, the other ENV params should be equal.

AFAIK it's confluent, not confluence.

👍 😄

WDYT about allowing non-confluent images
We add too much customization, if one wants to use his custom image, I would rather recommend him to build it on top of GenericContainer

kiview · 2018-01-17T14:03:00Z

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

+        withEnv("KAFKA_LOG_FLUSH_INTERVAL_MESSAGES", Long.MAX_VALUE + "");
+    }
+
+    public KafkaContainer withEmbeddedZookeeper() {


This works because confluent images contain zookeeper?

iNikem · 2018-01-21T11:43:56Z

Yo, @bsideup any estimation when this will be released? :)

bsideup · 2018-01-21T11:47:03Z

Hey @iNikem, I still wait for @rnorth's review, we're also running the tests in my company with KafkaContainer via JitPack, so far so good :)

rnorth · 2018-01-21T21:47:06Z

core/src/main/java/org/testcontainers/containers/SocatContainer.java

+
+    public SocatContainer() {
+        super(TestcontainersConfiguration.getInstance().getSocatContainerImage());
+        withCreateContainerCmdModifier(it -> it.withEntrypoint("/bin/sh"));


Just a thought - please could we set the container's name to something that people will be able to identify? (i.e. stop people wondering "what's this random socat container?"). A name like "testcontainers-socat-" + base58chars would probably work.

I realise that the image richnorth/ambassador or the random container names it was spawned with were never great in the first place, but we have a chance to do better 😄

rnorth

Only a few minor comments from me. I'm not a Kafka user so I put more weight on the fact that you and @kiview are using this already to know that it's good 😄

Please could you also add a suitable CHANGELOG entry and probably a docs update? Otherwise looks great to me!

…omment to KafkaContainer

bsideup · 2018-01-22T07:36:57Z

@rnorth done 👍

kiview · 2018-01-23T17:05:48Z

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

+        this("4.0.0");
+    }
+
+    public KafkaContainer(String confluentPlatformVersion) {


Maybe I missed something, but don't we need a custom wait strategy? Or is port based wait strategy already enough?

yes, Kafka is ready to be used when the port is open

I at least had some instances, where I got the following error when creating a topic:
ERROR org.apache.kafka.common.errors.InvalidReplicationFactorException: replication factor: 1 larger than available brokers: 0 (kafka.admin.TopicCommand$).

This did not happen for me, when I used
.waitingFor(new LogMessageWaitStrategy().withRegEx(/(?s).*started $kafka\.server\.KafkaServer$.*/))

"Started Kafka" != "Initialized Kafka", your clients should be able to sync the cluster state before doing operations (and most of them do).

Waiting for cluster initialization instead of just startup will just make the tests slower while it should be handled by your consumers / producers

I see, in this case we were using the bundled helper scripts, like kafka-topics.sh (but this is mostly a rudiment from having to use kafka with GenericContainer).

iNikem · 2018-01-24T09:38:50Z

Gentle push for @bsideup and @rnorth :)

* Add Kafka module * Replace AmbassadorContainer with SocatContainer in DockerComposeContainer * make it possible to use KafkaContainer with external Zookeeper * fix typo * fix Kafka tests * Add to CHANGELOG.md, name SocatContainer, add listeners explanation comment to KafkaContainer * listen on alias * rename myNetworkAlias -> networkAlias

Add Kafka module

b4ec814

bsideup added the enhancement label Jan 16, 2018

bsideup added this to the 1.6.0 milestone Jan 16, 2018

bsideup requested review from kiview and rnorth January 16, 2018 19:38

bsideup commented Jan 16, 2018

View reviewed changes

bsideup added 2 commits January 17, 2018 08:11

Replace AmbassadorContainer with SocatContainer in DockerComposeConta…

1c4e0df

…iner

make it possible to use KafkaContainer with external Zookeeper

5bb2e25

kiview requested changes Jan 17, 2018

View reviewed changes

bsideup added 2 commits January 17, 2018 15:17

fix typo

6a893dc

fix Kafka tests

47bedb6

kiview approved these changes Jan 18, 2018

View reviewed changes

rnorth reviewed Jan 21, 2018

View reviewed changes

rnorth requested changes Jan 21, 2018

View reviewed changes

Add to CHANGELOG.md, name SocatContainer, add listeners explanation c…

e93a7c1

…omment to KafkaContainer

kiview reviewed Jan 23, 2018

View reviewed changes

bsideup added 2 commits January 24, 2018 14:14

listen on alias

e02ab6e

rename myNetworkAlias -> networkAlias

b9608e3

rnorth approved these changes Jan 27, 2018

View reviewed changes

bsideup merged commit 8d5f7a9 into master Jan 27, 2018

bsideup deleted the kafka branch January 27, 2018 10:48

bsideup added the type/feature label Jan 28, 2018

Uh oh!

Comments

Conversation

bsideup commented Jan 16, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gAmUssA commented Jan 16, 2018

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!

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!

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!

iNikem commented Jan 21, 2018

Uh oh!

bsideup commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnorth left a comment

Choose a reason for hiding this comment

Uh oh!

bsideup commented Jan 22, 2018

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!

Choose a reason for hiding this comment

Uh oh!

iNikem commented Jan 24, 2018

Uh oh!