Skip to content

Conversation

JackyWoo
Copy link
Contributor

@JackyWoo JackyWoo commented Jan 5, 2023

isReusable should return a constant value.
Or ClickHouseHttpClient may close on flying connection when creating new connection.

if (connection != null && connection.isReusable()) {
closeConnection(connection, false);
}

@JackyWoo
Copy link
Contributor Author

JackyWoo commented Jan 5, 2023

When connection is on fly by a request the others wait it released?

@zhicwu
Copy link
Contributor

zhicwu commented Jan 5, 2023

Thanks for testing and raising the PR. ClickHouseHttpConnection.isReusable() suggests if the connection instance can be reused among requests. Each time when there's a new request made to the same server, we have two cases to deal with:

  1. reuse the same connection if it's not busy on something else
  2. create a new connection so that we can process requests in parallel

The change I made was for the latter one, which also fixed the test failures. Yes, it may cause connection leak but I thought the connections are all managed by Apache connection manager. Did you run into memory issue using latest code on develop branch? Will run stress test tonight to see how it goes.

@JackyWoo
Copy link
Contributor Author

JackyWoo commented Jan 5, 2023

@zhicwu We can use PoolingHttpClientConnectionManager and keep isReusable return true in which way connections will be managed by Apache connection manager.

@zhicwu
Copy link
Contributor

zhicwu commented Jan 5, 2023

Thank you @JackyWoo, the changes look good. I'm closing this PR as the changes are now merged into #1174, where I cleaned up code for consistency and improved compression/decompression support.

@zhicwu zhicwu closed this Jan 5, 2023
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.

2 participants