testcontainers-java icon indicating copy to clipboard operation
testcontainers-java copied to clipboard

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

Open codepitbull opened this issue 3 years ago • 3 comments

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

codepitbull avatar Apr 28 '21 20:04 codepitbull

@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?

bsideup avatar Sep 27 '21 10:09 bsideup

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 avatar Sep 29 '21 10:09 codepitbull

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

codepitbull avatar Sep 29 '21 12:09 codepitbull