clickhouse-java icon indicating copy to clipboard operation
clickhouse-java copied to clipboard

JDBC: max_open_connections doesn't limit the connections count

Open f1llon opened this issue 1 year ago • 7 comments

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

f1llon avatar Jun 21 '24 15:06 f1llon

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

  • setDefaultMaxPerRoute
  • setMaxTotal See https://hc.apache.org/httpcomponents-client-5.3.x/current/httpclient5/apidocs/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.html

I agree that documentation doesn't explain it.

chernser avatar Jul 01 '24 06:07 chernser

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.

f1llon avatar Jul 01 '24 07:07 f1llon

@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.

chernser avatar Jul 01 '24 15:07 chernser

Hi @chernser, I wanto to report another problem on Pool when settings the pool more than 10 max connections (im my test become more evident from 16 or more ) and create more concurrent requests to stress the pool (double of max connections), an errors like this become frequent :

[HikariPool-917 connection adder] ERROR com.clickhouse.client.http.ApacheHttpConnectionImpl -- HTTP request failed java.net.SocketException: No buffer space available (maximum connections reached?): connect

and using netstat I see many fewer active connections than requested. I'm using java 21.0.4 , Ch jdbc driver 0.6.4 Thanks a lot

quartex avatar Sep 06 '24 06:09 quartex

Hi, @quartex! what socket settings do you have? what is max connections? what is hikari pool settings? do you use hikari pool because use of JDBC?

Thanks!

chernser avatar Sep 06 '24 13:09 chernser

Hi @chernser, I wrote and performed the tests @quartex has mentioned. To answer you, I tried my benchmark stress tests using Hikari CP because we use Clickhouse in a microservice which already uses Hikari CP to manage its own connection pool, and we wanted to see the difference in performances between the default connection pool configuration (which unfortunately leads to the bug described in this issue) and a configuration that uses the already existent Hikari CP.

So I tried to execute parallel readings from a CH table and I had more issues when the size of my connection pool exceeded the max default value of 10 connections; below that value, even when the number of parallel readings was greater or equal to the Hikari maximumPoolSize property, my test was executed correctly. These are the results of my stress test with 8 parallel readings: image While with 16 or 12 as my connection pool size and executing 12 parallel readings I noticed the error message @quartex reported.

I set the connection pool size with statement:

HikariConfig config = new HikariConfig();
[..]
config.setMaximumPoolSize(Integer.parseInt(properties.getProperty("max_open_connections")));

where max_open_connections is the connectionPoolSize number you can see in the image. And even when I tried to execute 8 parallel readings with a pool of 5 connections, all 30 my tests where executed and the results registered by the benchmark.

The only Hikari properties I have are: image being maximumPoolSize the property I try to override.

And I don't have any explicit custom configuration for sockets, so I presume I am just using the default one for Spring Cloud 2023.0.2 + Jetty.

Thank you in advance!

emili4ciccone avatar Sep 09 '24 08:09 emili4ciccone

Hi @chernser ! Are there any news about the fixing of this bug? :)

emili4ciccone avatar Oct 15 '24 12:10 emili4ciccone

Good day, @emili4ciccone! It should be fixed. However I see there are mixed issues. Do you have multiple connection or instances of ClickHouseHttpConnection class?

Thanks!

chernser avatar Nov 06 '24 14:11 chernser

Hi @chernser, thank you for your attention! :)

Yes, the kind of tests we were conducting instantiate multiple connections, in order to monitor the performances when the number of parallel connections is higher than the capacity of the connection pool.

Anyway, I also tried running the sample project @f1llon provided, updating the clickhouse-jdbc driver to the latest 0.7.1 release, and I still see the same logs they reported in the original issue... :(

Thank you in advance!

emili4ciccone avatar Nov 08 '24 11:11 emili4ciccone

Hi @chernser, are there any news about this issue in version 0.7.2? Many thanks!

emili4ciccone avatar Jan 10 '25 13:01 emili4ciccone

Verified we actually honor this setting (ClientConfigProperties.HTTP_MAX_OPEN_CONNECTIONS) in V2

Paultagoras avatar May 14 '25 00:05 Paultagoras

Hi @Paultagoras, I have taken some new tests - I tried to re-run both my old tests and the project @f1llon provided - with clickhouse-jdbc version 0.8.6 and using JDBC-v2 classes (for example DataSourceImpl instead of the old ClickHouseDataSource). On one hand, I don't see anymore the Hikari CP issue I first submitted, so many thanks! But on the other hand, I still have some issues:

  • running @f1llon 's test project, I still have the old logs "route allocated: 1 of N; total allocated: 1 of N"
  • using the "default" connection pool and running my own tests of concurrent reading, property ClientConfigProperties.HTTP_MAX_OPEN_CONNECTIONS is ignored: when I run a netstat command on my PC, the number of connections opened toward ClickHouse can be well above the one I set with the ClientConfigProperties.HTTP_MAX_OPEN_CONNECTIONS property.

Is it enough to initialize my DataSourceImpl with baseProperties.setProperty(ClientConfigProperties.HTTP_MAX_OPEN_CONNECTIONS.getKey(), "5"); and then use datasource.getConnection() to make this configuration effective or do I need to do anything else?

Still, thank you for your help!

emili4ciccone avatar May 21 '25 14:05 emili4ciccone

@chernser

Paultagoras avatar May 21 '25 16:05 Paultagoras

Good day, @emili4ciccone!

I've created another issue to track the problem. https://github.com/ClickHouse/clickhouse-java/issues/2379

In general datasource connections is not the same as client max connections.

  • Java Client has a connection pool
  • JDBC connection in this case is an instance of a client
  • Datasources work with JDBC connections and is not aware about client instances.
  • When datasource asked for a connection it will create JDBC connection what will be one client instance with own connection pool

So in this case to match client per connection client should be configured as follows:

     .enableConnectionPool(false) // use basic connection pool. Creates connection each time
     .setMaxConnections(1) // to be sure no connection leak

chernser avatar May 21 '25 17:05 chernser

Hi @chernser , many thanks! Now it's clearer, I'll use the configuration you suggest for now and I'll keep on following the new issue!

emili4ciccone avatar May 22 '25 07:05 emili4ciccone