Skip to content

Avoid timezone query on create connection when not needed - #697

Closed
nicolasblaye wants to merge 4 commits into
ClickHouse:developfrom
nicolasblaye:master
Closed

nicolasblaye wants to merge 4 commits into
ClickHouse:developfrom
nicolasblaye:master

Conversation

@nicolasblaye

Copy link
Copy Markdown

This aims to prevent this query to append if we use the timezone specified in configuration.

@github-actions

Copy link
Copy Markdown
Benchmark                           (client)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc       normal  thrpt   20   252.204 ±  33.158  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20   242.658 ±  25.476  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc       normal  thrpt   20  1137.643 ± 180.147  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20  1151.844 ± 115.166  ops/s

@den-crane

Copy link
Copy Markdown
Contributor

But it skips to query version()

 if (serverVersion == null) {
            serverVersion = "";
        }

AFAIK it needs to implement lazy evaluation of the version?

@nicolasblaye

Copy link
Copy Markdown
Author

Looking at the code, it does need the version for one call.
I will add the possibility to specify the version too.

if (connection.getServerVersion().compareTo("1.1.54237") > 0) { query = new StringBuilder( "SELECT database, table, name, type, default_kind as default_type, default_expression, comment "); } else { query = new StringBuilder( "SELECT database, table, name, type, default_type, default_expression, NULL AS comment "); }

@github-actions

Copy link
Copy Markdown
Benchmark                           (client)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc       normal  thrpt   20   287.413 ±  29.619  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20   270.213 ±  32.679  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc       normal  thrpt   20  1797.617 ± 245.494  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20  2061.870 ± 224.084  ops/s

@github-actions

Copy link
Copy Markdown
Benchmark                           (client)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc       normal  thrpt   20   282.646 ±  23.716  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20   275.245 ±  25.968  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc       normal  thrpt   20  1785.376 ± 234.791  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20  1879.875 ± 255.126  ops/s

@den-crane

den-crane commented Jul 27, 2021 •

Copy link
Copy Markdown
Contributor

Looking at the code, it does need the version for one call.

version is used in tests. Your change skips tests / breaks CI.

@nicolasblaye

Copy link
Copy Markdown
Author

@den-crane Sorry I don't see which test fails. mvn test returns an unrelated NPE for me. How can I run them?

@den-crane

Copy link
Copy Markdown
Contributor

@nicolasblaye https://github.com/ClickHouse/clickhouse-jdbc/blob/master/clickhouse-jdbc/src/test/java/ru/yandex/clickhouse/integration/StreamSQLTest.java#L265

Some tests use getServerVersion() to make a decision does it need to run or not. Now they just skipped.

@nicolasblaye

Copy link
Copy Markdown
Author

@den-crane As far as I know, my last two commits should make so version is still fetched by default via a select. It shouldn't change the test no?

@den-crane

Copy link
Copy Markdown
Contributor

OK then. I did not check the last commit.

@zhicwu
zhicwu changed the base branch from master to develop July 28, 2021 02:16
@zhicwu

zhicwu commented Jul 28, 2021

Copy link
Copy Markdown
Contributor

Thanks for the pull request @nicolasblaye. It's one-time query for connection initialization, with trivial overhead IMO. Would you mind to share the problem you're trying to resolve here? From what I saw, disabling the query may help for unit testing, plus it will slightly increase throughput.

By the way, I changed target branch to develop. Could you check the build break as well as merge conflict so that we can merge?

@nicolasblaye

Copy link
Copy Markdown
Author

It's true that it's a small query, but we misconfigured our connection creation so we spam this query way too much (over a million time a day). Obviously, I am also fixing our connection creation issue but I figured I could also add that to the driver to avoid unnecessary (albeit very small) requests.

@github-actions

Copy link
Copy Markdown
Benchmark                           (client)  (connection)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc         reuse       normal  thrpt   20   262.416 ±  30.760  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc         reuse     prepared  thrpt   20   255.047 ±  26.253  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc           new       normal  thrpt   20   250.789 ±  32.380  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc           new     prepared  thrpt   20   251.747 ±  30.619  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc         reuse       normal  thrpt   20  1218.842 ± 131.862  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc         reuse     prepared  thrpt   20  1328.295 ± 176.315  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc           new       normal  thrpt   20  1223.360 ± 167.387  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc           new     prepared  thrpt   20  1212.160 ± 163.825  ops/s

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@zhicwu

zhicwu commented Nov 23, 2021

Copy link
Copy Markdown
Contributor

Won't merge this pull request as this has been addressed in new JDBC driver. When serverTimeZone and serverVersion are specified, the driver will no longer issue the query on creating connection.

@zhicwu zhicwu closed this Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spirngboot集成启动报错 Method threw java.lang.NullPointerException

4 participants