Skip to content

Prefer to use new ClickHouse JDBC driver#5666

Closed
pan3793 wants to merge 2 commits intotestcontainers:mainfrom
pan3793:patch-1
Closed

Prefer to use new ClickHouse JDBC driver#5666
pan3793 wants to merge 2 commits intotestcontainers:mainfrom
pan3793:patch-1

Conversation

@pan3793
Copy link
Copy Markdown
Contributor

@pan3793 pan3793 commented Aug 2, 2022

No description provided.

@pan3793 pan3793 requested a review from a team August 2, 2022 04:24
eddumelendez
eddumelendez previously approved these changes Aug 16, 2022
kiview
kiview previously approved these changes Aug 16, 2022
Copy link
Copy Markdown
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @pan3793.

@eddumelendez
Copy link
Copy Markdown
Member

@pan3793 thanks for the PR! but switching drivers seems to fail when com.clickhouse.jdbc.ClickHouseDriver is used against yandex/clickhouse-server:18.10.3. If the image is upgraded to, for example, yandex/clickhouse-server:21.3.20.1-alpine then succeeds. Due to we don't know which image versions are used, we can not accept the PR as-is for now. Unless , the image version is also take it into account in order to resolve the right driver.

@pan3793
Copy link
Copy Markdown
Contributor Author

pan3793 commented Aug 16, 2022

Thanks for review and tips, and also cc @zhicwu for suggestions since he is the clickhouse-jdbc maintainer

@eddumelendez eddumelendez removed this from the next milestone Aug 16, 2022
@zhicwu
Copy link
Copy Markdown

zhicwu commented Aug 16, 2022

Thanks @pan3793 for the PR!

Hi @eddumelendez, ClickHouse 18.x is not in active releases and it's no longer supported according to this. Can we upgrade image to clickhouse/clickhouse-server:21.8? I think we can drop the legacy driver, but it's fine to keep it as fallback in case there's work still rely on that.

@eddumelendez
Copy link
Copy Markdown
Member

@zhicwu regardless of what we are using internally, users can do new ClickHouseContainer("yandex/clickhouse-server:18.10.3") or new ClickHouseContainer("clickhouse/clickhouse-server:21.9.2-alpine") with any version and in order to provide a great experience and give the chance to users to upgrade on their own we need to manage this compatibility somehow. In the future, for a major release, we can drop any yandex reference :)

@eddumelendez
Copy link
Copy Markdown
Member

This was addressed already in #6236 depending on the image version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants