Replace deprecated driver ru.yandex.clickhouse.ClickHouseDriver by tangjiangling · Pull Request #10801 · trinodb/trino

tangjiangling · 2022-01-25T22:56:10Z

It is a continuation of #10559.

tangjiangling · 2022-01-25T23:08:59Z

Questions:

Does it make sense for the default value of clickhouse.use-deprecated-driver to be true?
Do we need to add documentation for the config clickhouse.use-deprecated-driver?
Do we need to check that the config clickhouse.use-deprecated-driver matches the version of clickhouse-jdbc? (e.g. clickhouse-jdbc version < 0.3.2, but clickhouse.use-deprecated-driver = false, which is wrong)

hashhar · 2022-01-27T07:32:46Z

Does it make sense for the default value of clickhouse.use-deprecated-driver to be true?

No. People generally don't change defaults and hence we'll never know if there are issues with the newer driver. We should default to newer driver and if people run into issues they have the escape hatch using the config.

Do we need to add documentation for the config clickhouse.use-deprecated-driver?

IMO we should.

Do we need to check that the config clickhouse.use-deprecated-driver matches the version of clickhouse-jdbc? (e.g. clickhouse-jdbc version < 0.3.2, but clickhouse.use-deprecated-driver = false, which is wrong)

The driver version is fixed at compile time so I don't see any way someone could load an older JDBC driver version. Can you expand a bit more on what scenario are you thinking of here?

tangjiangling · 2022-01-27T11:00:30Z

No. People generally don't change defaults and hence we'll never know if there are issues with the newer driver. We should default to newer driver and if people run into issues they have the escape hatch using the config.

I'm actually asking these questions because I want to know if I'm doing the right thing by adding the clickhouse.use-deprecated-driver configuration item.
As far as I know, the minimum supported ClickHouse version for Trino now is 20.3 (Altinity), so in order to be compatible with users who are still using the 20.3 (Alitinty) version, we need a configuration for users to choose from, but the problem is: if the default value is false, then users will report errors and complain during use, which is what I am worried about.

tangjiangling · 2022-01-27T11:06:07Z

if the default value is false, then users will report errors and complain during use

Why does the error occur?

Because the new driver does not support ClickHouse versions lower than 20.7 (you can find the ClickHouse versions supported by the new driver at https://github.com/ClickHouse/clickhouse-jdbc).

tangjiangling · 2022-01-27T11:26:09Z

The driver version is fixed at compile time so I don't see any way someone could load an older JDBC driver version. Can you expand a bit more on what scenario are you thinking of here?

Imagine if clickhouse.use-deprecated-driver is true (user can configure any value), which means you want to use the old Driver, but the clickhouse-jdbc version specified in pom.xml does not contain the old Driver (e.g. version=0.4.x), the error will occur at this time, so I'm wondering if I need to add such a check to prevent misconfiguration.

ebyhr · 2022-01-27T13:39:27Z

The driver version is fixed at compile time

@hashhar com.clickhouse:clickhouse-jdbc contains two drivers in a module and we're using deprecated one.

plugin/trino-clickhouse/pom.xml

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestingClickHouseServer.java

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClientModule.java

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

tangjiangling · 2022-01-31T13:03:46Z

Updated.

@ebyhr I found the unused property CLICKHOUSE_MAX_DECIMAL_PRECISION in ClickHouseClient, and by checking the git history, I found that you just deleted the reference to CLICKHOUSE_MAX_DECIMAL_PRECISION in this PR(#10703) without deleting its definition along with it.

https://github.com/trinodb/trino/blob/master/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java#L146

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClientModule.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/ClickHouseQueryRunner.java

...no-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorSmokeTest.java

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseTypeMapping.java

docs/src/main/sphinx/connector/clickhouse.rst

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseConfig.java

docs/src/main/sphinx/connector/clickhouse.rst

cla-bot · 2022-02-01T08:41:23Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

tangjiangling · 2022-02-01T08:50:57Z

@ebyhr Please recheck my CLA. I changed my github account name yesterday, now I changed it back.

ebyhr · 2022-02-01T08:56:35Z

@cla-bot check

cla-bot · 2022-02-01T08:56:38Z

The cla-bot has been summoned, and re-checked this pull request!

tangjiangling · 2022-02-01T11:37:48Z

https://github.com/trinodb/trino/runs/5018336836?check_suite_focus=true

Related issue #10871

tangjiangling · 2022-02-01T11:45:42Z

https://github.com/trinodb/trino/runs/5018336836?check_suite_focus=true
Related issue #10871

I'll update upstream.

ebyhr · 2022-02-01T11:49:56Z

Going to fix in #10886

tangjiangling · 2022-02-01T13:32:31Z

Related issue #10870

https://github.com/trinodb/trino/runs/5018336880?check_suite_focus=true

2022-02-01T11:52:31.0035961Z [ERROR] Tests run: 3094, Failures: 1, Errors: 0, Skipped: 92, Time elapsed: 2,550.964 s <<< FAILURE! - in TestSuite
2022-02-01T11:52:31.0037063Z [ERROR] io.trino.plugin.hive.TestOrcPageSourceMemoryTracking.testScanFilterAndProjectOperator  Time elapsed: 0.256 s  <<< FAILURE!
2022-02-01T11:52:31.0037799Z java.lang.AssertionError: expected [true] but found [false]
2022-02-01T11:52:31.0038199Z 	at org.testng.Assert.fail(Assert.java:94)
2022-02-01T11:52:31.0039237Z 	at org.testng.Assert.failNotEquals(Assert.java:513)
2022-02-01T11:52:31.0039642Z 	at org.testng.Assert.assertTrue(Assert.java:42)
2022-02-01T11:52:31.0040041Z 	at org.testng.Assert.assertTrue(Assert.java:52)
2022-02-01T11:52:31.0040942Z 	at io.trino.plugin.hive.TestOrcPageSourceMemoryTracking.testScanFilterAndProjectOperator(TestOrcPageSourceMemoryTracking.java:470)
2022-02-01T11:52:31.0041981Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2022-02-01T11:52:31.0042604Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2022-02-01T11:52:31.0043308Z 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2022-02-01T11:52:31.0044541Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2022-02-01T11:52:31.0045168Z 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
2022-02-01T11:52:31.0045750Z 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
2022-02-01T11:52:31.0046252Z 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
2022-02-01T11:52:31.0046796Z 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
2022-02-01T11:52:31.0047842Z 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
2022-02-01T11:52:31.0048429Z 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
2022-02-01T11:52:31.0049043Z 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2022-02-01T11:52:31.0049691Z 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2022-02-01T11:52:31.0050189Z 	at java.base/java.lang.Thread.run(Thread.java:829)
2022-02-01T11:52:31.0050392Z 
2022-02-01T11:52:31.0279744Z 2022-02-01T05:52:31.023-0600	INFO	Thread-5663	io.airlift.bootstrap.LifeCycleManager	JVM is shutting down, cleaning up
2022-02-01T11:52:31.0281732Z 2022-02-01T05:52:31.023-0600	INFO	Thread-5663	io.airlift.bootstrap.LifeCycleManager	Life cycle stopping...
2022-02-01T11:52:31.0282807Z 2022-02-01T05:52:31.023-0600	INFO	Thread-5663	io.airlift.bootstrap.LifeCycleManager	Life cycle stopped
2022-02-01T11:52:31.7275942Z [INFO] 
2022-02-01T11:52:31.7276579Z [INFO] Results:
2022-02-01T11:52:31.7277450Z [INFO] 
2022-02-01T11:52:31.7277806Z [ERROR] Failures: 
2022-02-01T11:52:31.7317526Z [ERROR]   TestOrcPageSourceMemoryTracking.testScanFilterAndProjectOperator:470 expected [true] but found [false]
2022-02-01T11:52:31.7321612Z [INFO] 
2022-02-01T11:52:31.7325713Z [ERROR] Tests run: 3094, Failures: 1, Errors: 0, Skipped: 92

Update:

I will update the upstream and re-test it.

The new ClickHouse Driver (`com.clickhouse.jdbc.ClickHouseDriver`) returns a different `jdbcTypeName` than the old ClickHouse Driver (`ru.yandex.clickhouse.ClickHouseDriver`). And we need to be compatible with the behavior of the new Driver.

The driver `ru.yandex.clickhouse.ClickHouseDriver` has been marked as deprecated. Also everything in package `ru.yandex.clickhouse` will be removed starting from 0.4.0.

tangjiangling · 2022-02-01T17:52:45Z

Going to fix in #10886

Updated upstream and re-pushed.

ebyhr · 2022-02-02T03:08:50Z

Merged, thanks!

tangjiangling · 2022-02-02T03:39:48Z

@ebyhr Thanks for your guidance!
Also thanks to @zhicwu, by the way, Happy Chinese New Year!

cla-bot bot added the cla-signed label Jan 25, 2022

tangjiangling requested a review from ebyhr January 25, 2022 22:56

tangjiangling mentioned this pull request Jan 25, 2022

Replace deprecated driver ru.yandex.clickhouse.ClickHouseDriver #10559

Closed

3 tasks

zhicwu mentioned this pull request Jan 29, 2022

Connection issue when there are several ClickHouse servers ClickHouse/clickhouse-java#822

Closed

ebyhr reviewed Jan 31, 2022

View reviewed changes

tangjiangling force-pushed the replace-clickhouse-driver branch from 5d430ee to b3df3b0 Compare January 31, 2022 12:41

github-actions bot added the docs label Jan 31, 2022

zhicwu mentioned this pull request Jan 31, 2022

Replace deprecated clickhouse driver testcontainers/testcontainers-java#4925

Closed

ebyhr approved these changes Feb 1, 2022

View reviewed changes

tangjiangling force-pushed the replace-clickhouse-driver branch from b3df3b0 to d42510b Compare February 1, 2022 08:41

cla-bot bot removed the cla-signed label Feb 1, 2022

cla-bot bot added the cla-signed label Feb 1, 2022

tangjiangling force-pushed the replace-clickhouse-driver branch from d42510b to 8f59446 Compare February 1, 2022 13:58

tangjiangling added 2 commits February 2, 2022 01:50

Bump clickhouse-jdbc version to 0.3.2-patch3

9985153

Replace deprecated driver ru.yandex.clickhouse.ClickHouseDriver

180e74b

The driver `ru.yandex.clickhouse.ClickHouseDriver` has been marked as deprecated. Also everything in package `ru.yandex.clickhouse` will be removed starting from 0.4.0.

tangjiangling force-pushed the replace-clickhouse-driver branch from 8f59446 to 180e74b Compare February 1, 2022 17:50

ebyhr merged commit d33de4f into trinodb:master Feb 2, 2022

ebyhr mentioned this pull request Feb 2, 2022

Release notes for 370 #10794

Closed

tangjiangling deleted the replace-clickhouse-driver branch February 2, 2022 03:30

github-actions bot added this to the 370 milestone Feb 2, 2022

mosabua mentioned this pull request Feb 2, 2022

Add Trino 370 release notes #10793

Merged

Conversation

tangjiangling commented Jan 25, 2022

Uh oh!

tangjiangling commented Jan 25, 2022

Uh oh!

hashhar commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tangjiangling commented Jan 27, 2022

Uh oh!

tangjiangling commented Jan 27, 2022

Uh oh!

tangjiangling commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebyhr commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangjiangling commented Jan 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cla-bot bot commented Feb 1, 2022

Uh oh!

tangjiangling commented Feb 1, 2022

Uh oh!

ebyhr commented Feb 1, 2022

Uh oh!

cla-bot bot commented Feb 1, 2022

Uh oh!

tangjiangling commented Feb 1, 2022

Uh oh!

tangjiangling commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebyhr commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tangjiangling commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tangjiangling commented Feb 1, 2022

Uh oh!

ebyhr commented Feb 2, 2022

Uh oh!

tangjiangling commented Feb 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

hashhar commented Jan 27, 2022 •

edited

Loading

tangjiangling commented Jan 27, 2022 •

edited

Loading

ebyhr commented Jan 27, 2022 •

edited

Loading

tangjiangling commented Feb 1, 2022 •

edited

Loading

ebyhr commented Feb 1, 2022 •

edited

Loading

tangjiangling commented Feb 1, 2022 •

edited

Loading