modules/clickhouse: JDBC driver name changed, liveness port#5474
modules/clickhouse: JDBC driver name changed, liveness port#5474eddumelendez merged 7 commits intotestcontainers:masterfrom anavrotski:clickhouse_driver_update
Conversation
…some minor changes
eddumelendez
left a comment
eddumelendez
left a comment
There was a problem hiding this comment.
thanks for your contribution @anavrotski ! the PR looks good to me, just wondering if we can make it compatible with both drivers? wdyt @kiview @bsideup ?
| this.withExposedPorts(HTTP_PORT, NATIVE_PORT); | ||
| this.waitingFor( |
There was a problem hiding this comment.
| this.withExposedPorts(HTTP_PORT, NATIVE_PORT); | |
| this.waitingFor( | |
| withExposedPorts(HTTP_PORT, NATIVE_PORT); | |
| waitingFor( |
There was a problem hiding this comment.
When I removed this. - IntelliJ Idea shows me a warning: 'SELF' used without 'try'-with-resources statement`
There was a problem hiding this comment.
About support of both drivers - Clickhouse is now separate company, not part of Yandex. And when old driver is used:
2022-06-06T18:45:27.806+0900 WARN main ru.yandex.clickhouse.ClickHouseDriver ******************************************************************************************
2022-06-06T18:45:27.806+0900 WARN main ru.yandex.clickhouse.ClickHouseDriver * This driver is DEPRECATED. Please use [com.clickhouse.jdbc.ClickHouseDriver] instead. *
2022-06-06T18:45:27.807+0900 WARN main ru.yandex.clickhouse.ClickHouseDriver * Also everything in package [ru.yandex.clickhouse] will be removed starting from 0.4.0. *
2022-06-06T18:45:27.807+0900 WARN main ru.yandex.clickhouse.ClickHouseDriver ******************************************************************************************
So I think its usage should be avoided.
There was a problem hiding this comment.
it complained about this. because the previous code for some reason did not pass a generic argument to JdbcDatabaseContainer, this should fix it:
https://github.com/testcontainers/testcontainers-java/pull/5474/files#r892189020
|
@eddumelendez I do see a problem with dropping the support for an old driver, as this is a breaking change. |
|
In order to keep backward compatibility we can do something like this @Override
public String getDriverClassName() {
try {
Class.forName("ru.yandex.clickhouse.ClickHouseDriver");
return "ru.yandex.clickhouse.ClickHouseDriver";
} catch (ClassNotFoundException e) {
return "com.clickhouse.jdbc.ClickHouseDriver";
}
} |
|
Why not try to load the new driver class first, and fall back to the old one if failed? |
|
@anavrotski by any chance do you have some time to work on the changes? Thanks in advance. |
…/ClickHouseContainer.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>
…/ClickHouseContainer.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>
4 participants
ru.yandex.clickhouse(deprected) tocom.clickhouseClickHouseContainerextends class with generic type (JdbcDatabaseContainer) I think it should extend it in same way asMySQLContainerorPostgreSQLContaineretc.getLivenessCheckPortNumberschanged to be the same as in containers mentioned above.