-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 module #546
Add Kafka module #546
Changes from 5 commits
b4ec814
1c4e0df
5bb2e25
6a893dc
47bedb6
e93a7c1
e02ab6e
b9608e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,7 +52,7 @@ class NetworkImpl extends ExternalResource implements Network { | |
private final AtomicBoolean initialized = new AtomicBoolean(); | ||
|
||
@Override | ||
public String getId() { | ||
public synchronized String getId() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nasty bug discovered during my experiments with Kafka module, concurrent access was giving There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tried making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a performance critical part (being called just a few times), doesn't make sense to over-optimize it :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Train your good habits! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keep it simple 😎 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't |
||
if (initialized.compareAndSet(false, true)) { | ||
id = create(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package org.testcontainers.containers; | ||
|
||
import org.testcontainers.utility.TestcontainersConfiguration; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
/** | ||
* A socat container is used as a TCP proxy, enabling any TCP port of another container to be exposed | ||
* publicly, even if that container does not make the port public itself. | ||
*/ | ||
public class SocatContainer extends GenericContainer<SocatContainer> { | ||
|
||
private final Map<Integer, String> targets = new HashMap<>(); | ||
|
||
public SocatContainer() { | ||
super(TestcontainersConfiguration.getInstance().getSocatContainerImage()); | ||
withCreateContainerCmdModifier(it -> it.withEntrypoint("/bin/sh")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I realise that the image |
||
} | ||
|
||
public SocatContainer withTarget(int exposedPort, String host) { | ||
return withTarget(exposedPort, host, exposedPort); | ||
} | ||
|
||
public SocatContainer withTarget(int exposedPort, String host, int internalPort) { | ||
addExposedPort(exposedPort); | ||
targets.put(exposedPort, String.format("%s:%s", host, internalPort)); | ||
return self(); | ||
} | ||
|
||
@Override | ||
protected void configure() { | ||
withCommand("-c", | ||
targets.entrySet().stream() | ||
.map(entry -> "socat TCP-LISTEN:" + entry.getKey() + ",fork,reuseaddr TCP:" + entry.getValue()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure that strange hostname values won't break the shell if we concat the String like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK any valid hostname should be valid shell argument |
||
.collect(Collectors.joining(" & ")) | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,10 @@ public String getAmbassadorContainerImage() { | |
return (String) properties.getOrDefault("ambassador.container.image", "richnorth/ambassador:latest"); | ||
} | ||
|
||
public String getSocatContainerImage() { | ||
return (String) properties.getOrDefault("socat.container.image", "alpine/socat:latest"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to use 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or would it be more responsible to maintain our own image? 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. once we get our name on Docker Hub - sure, for now I would stick with Alpine's |
||
} | ||
|
||
public String getVncRecordedContainerImage() { | ||
return (String) properties.getOrDefault("vncrecorder.container.image", "richnorth/vnc-recorder:latest"); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<parent> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>testcontainers-parent</artifactId> | ||
<version>0-SNAPSHOT</version> | ||
<relativePath>../../pom.xml</relativePath> | ||
</parent> | ||
|
||
<artifactId>kafka</artifactId> | ||
<name>TestContainers :: Apache Kafka</name> | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>${project.groupId}</groupId> | ||
<artifactId>testcontainers</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>org.apache.kafka</groupId> | ||
<artifactId>kafka-clients</artifactId> | ||
<version>1.0.0</version> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>org.assertj</groupId> | ||
<artifactId>assertj-core</artifactId> | ||
<version>3.8.0</version> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>com.google.guava</groupId> | ||
<artifactId>guava</artifactId> | ||
<version>23.0</version> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
</dependencies> | ||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
package org.testcontainers.containers; | ||
|
||
import org.testcontainers.utility.Base58; | ||
|
||
import java.util.stream.Stream; | ||
|
||
public class KafkaContainer extends GenericContainer<KafkaContainer> { | ||
|
||
public static final int KAFKA_PORT = 9092; | ||
|
||
public static final int ZOOKEEPER_PORT = 2181; | ||
|
||
protected String externalZookeeperConnect = null; | ||
|
||
protected SocatContainer proxy; | ||
|
||
public KafkaContainer() { | ||
this("4.0.0"); | ||
} | ||
|
||
public KafkaContainer(String confluentPlatformVersion) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I missed something, but don't we need a custom wait strategy? Or is port based wait strategy already enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, Kafka is ready to be used when the port is open There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I at least had some instances, where I got the following error when creating a topic: This did not happen for me, when I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, in this case we were using the bundled helper scripts, like |
||
super("confluentinc/cp-kafka:" + confluentPlatformVersion); | ||
|
||
withNetwork(Network.newNetwork()); | ||
withNetworkAliases("kafka-" + Base58.randomString(6)); | ||
withExposedPorts(KAFKA_PORT); | ||
|
||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!) ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
withEnv("KAFKA_OFFSETS_TOPIC_REPLICATION_FACTOR", "1"); | ||
withEnv("KAFKA_OFFSETS_TOPIC_NUM_PARTITIONS", "1"); | ||
withEnv("KAFKA_LOG_FLUSH_INTERVAL_MESSAGES", Long.MAX_VALUE + ""); | ||
} | ||
|
||
public KafkaContainer withEmbeddedZookeeper() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works because confluent images contain zookeeper? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
externalZookeeperConnect = null; | ||
return self(); | ||
} | ||
|
||
public KafkaContainer withExternalZookeeper(String connectString) { | ||
externalZookeeperConnect = connectString; | ||
return self(); | ||
} | ||
|
||
public String getBootstrapServers() { | ||
return String.format("PLAINTEXT://%s:%s", proxy.getContainerIpAddress(), proxy.getFirstMappedPort()); | ||
} | ||
|
||
@Override | ||
public void start() { | ||
proxy = new SocatContainer() | ||
.withNetwork(getNetwork()) | ||
.withTarget(9092, getNetworkAliases().get(0)) | ||
.withTarget(2181, getNetworkAliases().get(0)); | ||
|
||
proxy.start(); | ||
withEnv("KAFKA_ADVERTISED_LISTENERS", "BROKER://127.0.0.1:9093,PLAINTEXT://" + proxy.getContainerIpAddress() + ":" + proxy.getFirstMappedPort()); | ||
|
||
if (externalZookeeperConnect != null) { | ||
withEnv("KAFKA_ZOOKEEPER_CONNECT", externalZookeeperConnect); | ||
} else { | ||
addExposedPort(ZOOKEEPER_PORT); | ||
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"); | ||
} | ||
|
||
super.start(); | ||
} | ||
|
||
@Override | ||
public void stop() { | ||
Stream.<Runnable>of(super::stop, proxy::stop).parallel().forEach(Runnable::run); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
clientPort=2181 | ||
dataDir=/var/lib/zookeeper/data | ||
dataLogDir=/var/lib/zookeeper/log |
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.
Shouldn't this changes happen in a different PR? It's a quite big one.
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.
there is little to no activity around DockerComposeContainer, so I would keep it as part of this PR until you have objections :)
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'll try this change on Windows tonight 😉
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.
All good on Docker for Windows 💪