search

ClickHouse / clickhouse-java

Java client and JDBC driver for ClickHouse
https://clickhouse.com
Apache License 2.0
1.39k stars 513 forks source link

JDBC: max_open_connections doesn't limit the connections count #1705

Open f1llon opened 1 week ago

f1llon commented 1 week ago

Describe the bug

It seems that max_open_connections config option doesn't work as expected - it doesn't limit the number of opened connections.

As far from I see from the driver codebase, a new instance of the ApacheHttpConnectionImpl is created for each connection and the config option is used inside. Thus it has no impact on the real number of opened connections.

Steps to reproduce

  1. create ClickHouseDataSource with MAX_OPEN_CONNECTIONS property
  2. execute multiple (more than MAX_OPEN_CONNECTIONS value) select statements in parallel
  3. all the requests are executed in parallel, MAX_OPEN_CONNECTIONS has no impact on it

Expected behavior

the number of parallel requests is limited by MAX_OPEN_CONNECTIONS, other requests are waiting in a queue

Code example

see full example here

Error log

see log here

take a look at messages like - "[total available: 0; route allocated: 1 of 3; total allocated: 1 of 3]" it's always 1 of N

Dependencies

see project dependencies here

chernser commented 2 days ago

Good day, @f1llon ! This option sets apache client pool configuration:

I agree that documentation doesn't explain it.

f1llon commented 2 days ago

Hey, @chernser. Thank you for your attention to this issue!

As I mentioned, a new instance of the ApacheHttpConnectionImpl is created for each connection. A new PoolingHttpClientConnectionManager is created under the hood. The MAX_OPEN_CONNECTIONS property is used to setMaxTotal and setDefaultMaxPerRoute, see sources.

Thus, we have the same number of PoolingHttpClientConnectionManagers as many connections we have.

It seems more logical to me to set up a singleton PoolingHttpClientConnectionManager and reuse it for each connection. Or maybe I misunderstood something.

chernser commented 2 days ago

@f1llon you are right. I just wanted to mention that initial purpose of the setting MAX_OPEN_CONNECTIONS is to pass configuration to the Apache HC. Current behavior of the CH Client is not correct and we will fix it.