Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions modules/clickhouse/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,6 @@ dependencies {

testImplementation project(':jdbc-test')
testImplementation 'ru.yandex.clickhouse:clickhouse-jdbc:0.3.1-patch'
testImplementation 'mysql:mysql-connector-java:8.0.24'
testImplementation 'io.vertx:vertx-mysql-client:4.0.2'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.testcontainers.containers;

import org.testcontainers.utility.DockerImageName;

public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {
abstract class AbstractClickHouseContainerJdbc extends JdbcDatabaseContainer {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


public AbstractClickHouseContainerJdbc(final DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(ClickHouseInit.DEFAULT_IMAGE_NAME);
ClickHouseInit.Init(this);
}

@Override
protected Integer getLivenessCheckPort() {
return getMappedPort(ClickHouseInit.HTTP_PORT);
}

@Override
public String getDriverClassName() {
return ClickHouseInit.MYSQL_DRIVER_CLASS_NAME;
}

@Override
public String getDatabaseName() {
return ClickHouseInit.databaseName;
}

@Override
public String getUsername() {
return ClickHouseInit.username;
}

@Override
public String getPassword() {
return ClickHouseInit.password;
}

@Override
public String getTestQueryString() {
return ClickHouseInit.TEST_QUERY;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, good point.

}
}
Original file line number Diff line number Diff line change
@@ -1,92 +1,45 @@
package org.testcontainers.containers;

import com.google.common.collect.Sets;
import org.testcontainers.containers.wait.strategy.HttpWaitStrategy;
import org.testcontainers.utility.DockerImageName;

import java.time.Duration;
import java.util.Set;

public class ClickHouseContainer extends JdbcDatabaseContainer {
public static final String NAME = "clickhouse";

private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("yandex/clickhouse-server");

@Deprecated
public static final String IMAGE = DEFAULT_IMAGE_NAME.getUnversionedPart();

@Deprecated
public static final String DEFAULT_TAG = "18.10.3";

public static final Integer HTTP_PORT = 8123;
public static final Integer NATIVE_PORT = 9000;
public class ClickHouseContainer<SELF extends ClickHouseContainer<SELF>> extends GenericContainer<SELF> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static final String DRIVER_CLASS_NAME = "ru.yandex.clickhouse.ClickHouseDriver";
private static final String JDBC_URL_PREFIX = "jdbc:" + NAME + "://";
private static final String TEST_QUERY = "SELECT 1";

private String databaseName = "default";
private String username = "default";
private String password = "";

/**
* @deprecated use {@link ClickHouseContainer(DockerImageName)} instead
*/
@Deprecated
public ClickHouseContainer() {
this(DEFAULT_IMAGE_NAME.withTag(DEFAULT_TAG));
this(ClickHouseInit.DEFAULT_TAG);
}

public ClickHouseContainer(String dockerImageName) {
this(DockerImageName.parse(dockerImageName));
public ClickHouseContainer(String tag) {
this(ClickHouseInit.DEFAULT_IMAGE_NAME.withTag(tag));
}

public ClickHouseContainer(final DockerImageName dockerImageName) {
public ClickHouseContainer(DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(ClickHouseInit.DEFAULT_IMAGE_NAME);

dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);

withExposedPorts(HTTP_PORT, NATIVE_PORT);
waitingFor(
new HttpWaitStrategy()
.forStatusCode(200)
.forResponsePredicate(responseBody -> "Ok.".equals(responseBody))
.withStartupTimeout(Duration.ofMinutes(1))
);
ClickHouseInit.Init(this);
}

@Override
public Set<Integer> getLivenessCheckPortNumbers() {
return Sets.newHashSet(HTTP_PORT);
return Sets.newHashSet(ClickHouseInit.HTTP_PORT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Override
public String getDriverClassName() {
return DRIVER_CLASS_NAME;
public String getDatabaseName() {
return ClickHouseInit.databaseName;
}

@Override
public String getJdbcUrl() {
return JDBC_URL_PREFIX + getHost() + ":" + getMappedPort(HTTP_PORT) + "/" + databaseName;
}

@Override
public String getUsername() {
return username;
return ClickHouseInit.username;
}

@Override
public String getPassword() {
return password;
return ClickHouseInit.password;
}

@Override
public String getTestQueryString() {
return TEST_QUERY;
return ClickHouseInit.TEST_QUERY;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it


@Override
public ClickHouseContainer withUrlParam(String paramName, String paramValue) {
throw new UnsupportedOperationException("The ClickHouse does not support this");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.testcontainers.containers;

import org.testcontainers.utility.DockerImageName;

public class ClickHouseContainerJdbcMysql extends AbstractClickHouseContainerJdbc {

public ClickHouseContainerJdbcMysql(DockerImageName dockerImageName) {
super(dockerImageName);
}

@Override
public String getDriverClassName() {
return ClickHouseInit.MYSQL_DRIVER_CLASS_NAME;
}

@Override
public String getJdbcUrl() {
return "jdbc:mysql://" + getHost() + ":" + getMappedPort(ClickHouseInit.MYSQL_PORT) + "/" + getDatabaseName() + "?user=" + getUsername() + "&password=" + getPassword();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.testcontainers.containers;

import org.testcontainers.utility.DockerImageName;

import static org.testcontainers.containers.ClickHouseInit.HTTP_PORT;

public class ClickHouseContainerJdbcYandex extends ClickHouseContainerJdbcMysql {

public ClickHouseContainerJdbcYandex(DockerImageName dockerImageName) {
super(dockerImageName);
}

@Override
public String getDriverClassName() {
return ClickHouseInit.CLICKHOUSE_DRIVER_CLASS_NAME;
}

@Override
public String getJdbcUrl() {
return "jdbc:clickhouse://" + getHost() + ":" + getMappedPort(HTTP_PORT) + "/" + getDatabaseName() + "?user=" + getUsername() + "&password=" + getPassword();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.testcontainers.containers;

import org.testcontainers.containers.wait.strategy.HttpWaitStrategy;
import org.testcontainers.utility.DockerImageName;

import java.time.Duration;

public class ClickHouseInit {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@codepitbull codepitbull Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #4038 (comment) will make it obsolete

public static final String JDBC_NAME_YANDEX = "clickhouse";
public static final String JDBC_NAME_MYSQL = "mysql";

public static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("yandex/clickhouse-server");

public static final String CLICKHOUSE_DRIVER_CLASS_NAME = "ru.yandex.clickhouse.ClickHouseDriver";
public static final String MYSQL_DRIVER_CLASS_NAME = "com.mysql.cj.jdbc.Driver";

public static final Integer HTTP_PORT = 8123;
public static final Integer NATIVE_PORT = 9000;
public static final Integer MYSQL_PORT = 9004;

public static final String databaseName = "default";
public static final String username = "default";
public static final String password = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't those uppercase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


static final String TEST_QUERY = "SELECT 1";

public static final String DEFAULT_TAG = "21.3.8.76";

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't do this in Java =P

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

container.withExposedPorts(HTTP_PORT, NATIVE_PORT, MYSQL_PORT);

container.waitingFor(
new HttpWaitStrategy()
.forStatusCode(200)
.forPort(HTTP_PORT)
.forResponsePredicate(responseBody -> "Ok.".equals(responseBody))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.withStartupTimeout(Duration.ofMinutes(1))
);
}
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package org.testcontainers.containers;

import org.testcontainers.utility.DockerImageName;

public class ClickHouseProvider extends JdbcDatabaseContainerProvider {
@Override
public boolean supports(String databaseType) {
return databaseType.equals(ClickHouseContainer.NAME);
return databaseType.equals(ClickHouseInit.JDBC_NAME_YANDEX) || databaseType.equals(ClickHouseInit.JDBC_NAME_MYSQL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

@Override
public JdbcDatabaseContainer newInstance() {
return newInstance(ClickHouseInit.DEFAULT_TAG);
}

@Override
public JdbcDatabaseContainer newInstance(String tag) {
return new ClickHouseContainer(DockerImageName.parse(ClickHouseContainer.IMAGE).withTag(tag));
return new ClickHouseContainerJdbcYandex(ClickHouseInit.DEFAULT_IMAGE_NAME.withTag(tag));
}
}
Original file line number Diff line number Diff line change
@@ -1 +1 @@
org.testcontainers.containers.ClickHouseProvider
org.testcontainers.containers.ClickHouseProvider
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
import org.testcontainers.utility.DockerImageName;

public interface ClickhouseTestImages {
DockerImageName CLICKHOUSE_IMAGE = DockerImageName.parse("yandex/clickhouse-server:18.10.3");
DockerImageName CLICKHOUSE_IMAGE = DockerImageName.parse("yandex/clickhouse-server:21.3.8.76");
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public static Iterable<Object[]> data() {
return asList(
new Object[][]{
{"jdbc:tc:clickhouse://hostname/databasename", EnumSet.of(Options.PmdKnownBroken)},
//Not testing jdbc:tc:mysql here because the connection pool used by AbstractJDBCDriverTest tries
//to several things that aren't currently support by the clickhouse-mysql-impl
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package org.testcontainers.junit.clickhouse;

import org.junit.Test;
import org.testcontainers.containers.ClickHouseContainerJdbcMysql;

import java.sql.Connection;
import java.sql.SQLException;

import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue;
import static org.testcontainers.ClickhouseTestImages.CLICKHOUSE_IMAGE;

public class SimpleClickhouseJdbcMysqlTest {

@Test
public void testSimple() throws SQLException {
try (ClickHouseContainerJdbcMysql clickhouse = new ClickHouseContainerJdbcMysql(CLICKHOUSE_IMAGE)) {
clickhouse.start();

try (Connection connection = clickhouse.createConnection("")) {
boolean testQuerySucceeded = connection.createStatement().execute("SELECT 1");
assertTrue("A basic SELECT query succeeds", testQuerySucceeded);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.testcontainers.junit.clickhouse;

import org.junit.Test;
import org.testcontainers.containers.ClickHouseContainerJdbcYandex;
import org.testcontainers.db.AbstractContainerDatabaseTest;

import java.sql.ResultSet;
import java.sql.SQLException;

import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;
import static org.testcontainers.ClickhouseTestImages.CLICKHOUSE_IMAGE;

public class SimpleClickhouseJdbcYandexTest extends AbstractContainerDatabaseTest {

@Test
public void testSimple() throws SQLException {
try (ClickHouseContainerJdbcYandex clickhouse = new ClickHouseContainerJdbcYandex(CLICKHOUSE_IMAGE)) {
clickhouse.start();

ResultSet resultSet = performQuery(clickhouse, "SELECT 1");

int resultSetInt = resultSet.getInt(1);
assertEquals("A basic SELECT query succeeds", 1, resultSetInt);
}
}
}
Loading