Implement Clickhouse containers for JDBC (mysql/Yandex) and Mysql-binary-access #4038

codepitbull · 2021-04-28T20:38:08Z

Why ?

The current implementation of the Clickhouse-Container only supports the Yandex-JDBC-driver and enforces it to be on the classpath.
It also doesn't export trhe MYSQL-port available in Clickhouse.
The base image includedd is also relying on an old version of Clickhouse that was missing some compatibility fixes for the MySQL-JDBC-driver.

Since I also wanted to test the Vert.x MySQL-driver against Clickhouse I needed to fix these things.

What ?

This PR provides the following things:

ClickHouseContainerJdbcMysql for using the MySQL-JDBC-driver (relys on the MySQL-port being exposed and the driver being on the classpath)
ClickHouseContainerJdbcYandex for using hte Yandeex-JDBC-driver (relys on the HTTP-port being exposed and the driver being on the classpath)
ClickHouseContainer for accessing the HTTP and MYSQL-ports without having to rely on the presence of one of the JDBC-drivers

…Mysql-binary-access

kiview · 2021-09-23T14:40:57Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseContainer.java

-
-    public static final Integer HTTP_PORT = 8123;
-    public static final Integer NATIVE_PORT = 9000;
+public class ClickHouseContainer<SELF extends ClickHouseContainer<SELF>> extends GenericContainer<SELF> {


It's okay and more consistent with other modern modules to leave out the generic type here.

Also, removing extends JdbcDatabaseContainer is a breaking change - please revert

This is probably a naming messup on my side.
There are now 3 containers:
ClickHouseContainerJdbcMysql (extends JdbcDatabaseContainer) => Requires the mysql-driver to work
ClickHouseContainerJdbcYandex (extends JdbcDatabaseContainer) => Requires the yandex-driver to work
ClickHouseContainer => Not a JDBC container since it is used to test via the raw wire protocl (mysql) and the http api, this shouldn't rely on having a JDBC driver on the path.

What would you suggest naming wise?

The naming is okay, just we can't drop the parent class like that - not everyone starts containers with the JDBC URL integration, some may have used it as var ch = new ClickHouseContainer(); ch.start(); ch.getJdbcUrl()

kiview

Hey @codepitbull, sorry for taking our time with this PR.

PR looks pretty good, just having some general remarks.
I was wondering if the 3 different classes like this are really necessary, or if we couldn't just use a single class and its type configurable (with the type just resulting in a different value for driverClassName after all). But maybe I am missing something, I have no experience with Clickhouse and look at it purely from the Testcontainers perspective.

kiview · 2021-09-23T14:50:12Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseContainer.java

    @Override
    public Set<Integer> getLivenessCheckPortNumbers() {
-        return Sets.newHashSet(HTTP_PORT);
+        return Sets.newHashSet(ClickHouseInit.HTTP_PORT);


Suggested change

return Sets.newHashSet(ClickHouseInit.HTTP_PORT);

return Collections.singleton(ClickHouseInit.HTTP_PORT);

kiview · 2021-09-23T15:06:43Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseContainer.java

    public String getTestQueryString() {
-        return TEST_QUERY;
+        return ClickHouseInit.TEST_QUERY;
    }


This method is not necessary anymore if the class does not extend JdbcContainer anymore. Maybe we deprecate it in this PR or remove it?

kiview · 2021-09-23T15:08:35Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseInit.java

+    public static final String databaseName = "default";
+    public static final String username = "default";
+    public static final String password = "";


Why aren't those uppercase?

Because I have been doing Go for too long ... geez, how stuff like that seeps into daily coding.
Will change.

kiview · 2021-09-23T15:09:13Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseInit.java

+
+import java.time.Duration;
+
+public class ClickHouseInit {


In this class, we should only make the constants public that need to be public and keep the rest package-protected. The class itself can also be package-protected, or?

Main reason this class isn't package-protected is that I was trying to follow the package layout you picked which moves the junit-tests into a dedicated folder.
From there I can't access the class to get the constants for testing.
If you are ok with merging the packages I can make it protected.

I think #4038 (comment) will make it obsolete

kiview · 2021-09-23T15:09:38Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseInit.java

+            new HttpWaitStrategy()
+                .forStatusCode(200)
+                .forPort(HTTP_PORT)
+                .forResponsePredicate(responseBody -> "Ok.".equals(responseBody))


Is this response important if we already get 200 status code?

I prefer to be strict with these (bad experiences with APIs returning 200 but with a failure message).
In our own driver we check for the "Ok." reply since it is explicitly mentioned in the clickhouse docs as the return value that indicates success.

I am not sure I share the preference :) It seems to be a free text value, not even some structured response (e.g. JSON).

I would rely on ClickHouse devs being the good folks who understand the meaning of 200 and would never return 200 in case of an error :)

kiview · 2021-09-23T15:12:26Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseProvider.java

    @Override
    public boolean supports(String databaseType) {
-        return databaseType.equals(ClickHouseContainer.NAME);
+        return databaseType.equals(ClickHouseInit.JDBC_NAME_YANDEX) || databaseType.equals(ClickHouseInit.JDBC_NAME_MYSQL);


Wouldn't this collide with the normal mysql JDBC URL if someone has both modules on the classpath? In this case, they would get either container.

It would be interesting to see what happens in this case, but I think it's a good point. I wonder if we should:
(a) make the ContainerDatabaseDriver fail fast in this scenario, or
(b) require a different string to be used, e.g. tc:clickhousemysql. This seems less nice though, as it breaks an implicit contract about use of tc: in URLs.

While (a) sounds reasonable (and I wonder why we decided to return any matching container in case of collision) how would a user that has both dependencies on their classpath (for a valid reason) deal with this? Our tests would also fail, or?

kiview · 2021-09-23T15:13:38Z

modules/clickhouse/src/test/java/org/testcontainers/junit/clickhouse/SimpleClickhouseTest.java

-            clickhouse.start();
+    public void testRawMysql() throws Throwable {
+        ClickHouseContainer clickHouseContainer = new ClickHouseContainer();
+        clickHouseContainer.start();


Why did you remove the try-with-resource? We use this to close the container after the test.

kiview · 2021-09-23T15:14:41Z

modules/clickhouse/src/test/java/org/testcontainers/junit/clickhouse/SimpleClickhouseTest.java


-            ResultSet resultSet = performQuery(clickhouse, "SELECT 1");
+        MySQLConnectOptions connectOptions = new MySQLConnectOptions()
+            .setPort(clickHouseContainer.getMappedPort(ClickHouseInit.MYSQL_PORT))


I feel like having the constant in ClickHousInit makes it a bit harder to discover for users.

The constants are shared between all three.

What do you think about adding this to ClickHouseContainer?

public Integer getHttpPort() { return getMappedPort(ClickHouseInit.HTTP_PORT); } public Integer getNativePort() { return getMappedPort(ClickHouseInit.NATIVE_PORT); } public Integer getMysqlPort() { return getMappedPort(ClickHouseInit.MYSQL_PORT); }

bsideup · 2021-09-27T10:12:23Z

modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseInit.java

+
+    public static final String DEFAULT_TAG = "21.3.8.76";
+
+    public static void Init(GenericContainer<?> container) {


we don't do this in Java =P

Suggested change

public static void Init(GenericContainer<?> container) {

public static void init(GenericContainer<?> container) {

bsideup · 2021-09-27T10:12:49Z

.../clickhouse/src/main/java/org/testcontainers/containers/AbstractClickHouseContainerJdbc.java

+
+import org.testcontainers.utility.DockerImageName;
+
+public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {


Suggested change

public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {

abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {

bsideup · 2021-09-27T10:18:49Z

.../clickhouse/src/main/java/org/testcontainers/containers/AbstractClickHouseContainerJdbc.java

+
+    @Override
+    public AbstractClickHouseContainerJdbc withUrlParam(String paramName, String paramValue) {
+        throw new UnsupportedOperationException("The ClickHouse does not support this");


I think it does support a couple of parameters, btw :D Perhaps this PR is a nice opportunity to fix this 👍

https://github.com/ClickHouse/clickhouse-jdbc/blob/5e6f74b89354e60af7d0c9437eef0520ee9ffda1/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseProperties.java#L106

Ahh, yes, good point.

bsideup · 2021-09-27T10:25:33Z

@codepitbull after reviewing the PR, I got a feeling that it can be done in a much easier way. We could keep the original ClickHouseContainer and have a couple of "proxy" classes that will take ClickHouseContainer as an argument and implement the required methods.

The getJdbcUrl() can either be overloaded with enum Flavor {MYSQL,CLICKHOUSE} or we could add getMySQLJdbcUrl() or similar method.

This way we can dramatically reduce the size of this PR, avoid breaking changes and keep it simple (eg no need for ClickHouseInit at all). WDYT?

codepitbull · 2021-09-29T10:57:14Z

Hey @codepitbull, sorry for taking our time with this PR.

PR looks pretty good, just having some general remarks. I was wondering if the 3 different classes like this are really necessary, or if we couldn't just use a single class and its type configurable (with the type just resulting in a different value for driverClassName after all). But maybe I am missing something, I have no experience with Clickhouse and look at it purely from the Testcontainers perspective.

@kiview see my answer in the other thread:
Short version:
ClickHouseContainer is using the raw protocols and should not rely on having a JDBC-driver on the classpath.
For the other two it's a question of style and I will rework it as you need it. My personal opinion on this is that I prefer separate classes because they rely on different JDBC-drivers. I am not a huge fan to have this configurable, that's why I moved all shared code into AbstractClickHouseContainerJdbc.

codepitbull · 2021-09-29T12:22:54Z

@codepitbull after reviewing the PR, I got a feeling that it can be done in a much easier way. We could keep the original ClickHouseContainer and have a couple of "proxy" classes that will take ClickHouseContainer as an argument and implement the required methods.

The getJdbcUrl() can either be overloaded with enum Flavor {MYSQL,CLICKHOUSE} or we could add getMySQLJdbcUrl() or similar method.

This way we can dramatically reduce the size of this PR, avoid breaking changes and keep it simple (eg no need for ClickHouseInit at all). WDYT?

Should be possible, but it will take a while until I get to this.

eddumelendez · 2025-12-15T16:41:25Z

Closing due to there is no progress on this.

Implement separate Clickhouse containers for JDBC (mysql/Yandex) and …

9d57fa2

…Mysql-binary-access

codepitbull requested review from bsideup, kiview and rnorth as code owners April 28, 2021 20:38

Better naming for the tests and some docs

c767d86

codepitbull mentioned this pull request Apr 28, 2021

Clickhouse via MySQL protocol #3827

Open

Merge with master

1deee89

kiview reviewed Sep 23, 2021

View reviewed changes

kiview requested changes Sep 23, 2021

View reviewed changes

bsideup reviewed Sep 27, 2021

View reviewed changes

codepitbull added 2 commits September 29, 2021 12:51

Removed generic type from ClickhouseContainer

18c8f96

Removed unused getTestQueryString-method from ClickHouseContainer

3ab7757

codepitbull added 4 commits September 29, 2021 12:59

Code style

4e03f66

SimpleClickhouseTest now using try-with-resource

e0c02d6

Lowercase init ...

ab32105

made AbstractClickHouseContainerJdbc package-protected

9f615fe

eddumelendez assigned eddumelendez and kiview Jul 26, 2022

eddumelendez requested a review from a team November 19, 2024 22:04

eddumelendez requested a review from a team as a code owner August 19, 2025 16:51

eddumelendez force-pushed the main branch from b45c488 to 5f96a1b Compare December 2, 2025 21:09

eddumelendez closed this Dec 15, 2025

	return Sets.newHashSet(ClickHouseInit.HTTP_PORT);
	return Collections.singleton(ClickHouseInit.HTTP_PORT);


		public static final String DEFAULT_TAG = "21.3.8.76";

		public static void Init(GenericContainer<?> container) {


		import org.testcontainers.utility.DockerImageName;

		public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {

	public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {
	abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {

Uh oh!

Implement Clickhouse containers for JDBC (mysql/Yandex) and Mysql-binary-access #4038

Implement Clickhouse containers for JDBC (mysql/Yandex) and Mysql-binary-access #4038

Uh oh!

Conversation

codepitbull commented Apr 28, 2021

Why ?

What ?

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!

kiview left a comment

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!

codepitbull Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

bsideup commented Sep 27, 2021

Uh oh!

codepitbull commented Sep 29, 2021

Uh oh!

codepitbull commented Sep 29, 2021

Uh oh!

codepitbull Sep 29, 2021 •

edited

Loading