Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

define couchDB connection pool size #654

Merged

Conversation

cendhu
Copy link
Contributor

@cendhu cendhu commented Feb 11, 2020

Type of change

  • Bug fix
  • Improvement (improvement to code, performance, etc)

Description

When the CouchDB is used as the StateDB, we observe too many TCP connections at the time_wait state.

$ netstat -n | grep -i 5984 | grep -i time_wait | wc -l
34000

As the /proc/sys/net/ipv4/tcp_fin_timeout (i.e., keep-alive timeout) is usually set to 60 seconds in the Linux server, we quickly get into the following error when the peer tries to access the CouchDB:

connect: cannot assign requested address

Reason for the above behavior. The peer creates a large number of TCP connections to CouchDB because the default reusable connection pool size is 2 per host while the max pool size is 100 (combining all hosts) -- refer to transport.go and golang http doc. Thus only 2 TCP connections can be reused by the peer when it accesses CouchDB. When more than two goroutines in the peer try to simultaneously access the CouchDB, it results in new non-reusable connections (i.e., creation of new TCP connection outside the pool). Hence, when the peer runs for around 30 seconds at a high load, it results in too many TCP connections at the time_wait state.

Fix: We set MaxIdleConns and MaxIdleConnsPerHost to 2000. This would create 2000 TCP connections in the pool and it would be reused whenever the peer accesses the CouchDB. As the peer is the only host for the CouchDB, we set the same value for both parameters.

Additional details

We tested it using a load generator by generating 700 tps. Now, there is no TCP connection at the time_wait state. This PR is cherry-picked from the master -- #429

Related issues

FAB-17277

FAB-17277 fix too many TCP connections between peer and CouchDB

Increase the connection pool size per host from 2 to 2000.
Also, increase the total connection pool size to 2000.

Signed-off-by: Senthil Nathan N <cendhu@gmail.com>
(cherry picked from commit 28c6efd)
@cendhu cendhu requested a review from a team as a code owner February 11, 2020 06:50
@denyeart denyeart merged commit 9faad9e into hyperledger:release-1.4 Feb 11, 2020
@@ -48,8 +49,20 @@ func CreateCouchInstance(couchDBConnectURL, id, pw string, maxRetries,
// and for efficiency should only be created once and re-used.
client := &http.Client{Timeout: couchConf.RequestTimeout}

transport := &http.Transport{Proxy: http.ProxyFromEnvironment}
transport.DisableCompression = false
transport := &http.Transport{
Copy link
Contributor

@manish-sethi manish-sethi Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As, it is merged in master, I am fine with this backport. However, just to take an additional note, I would have preferred this change the following way...

      transport := http.DefaultTransport.(*http.Transport).Clone()
      transport.MaxIdleConns = 2000
      transport.MaxIdleConnsPerHost = 2000

As, this makes it easy to read what is changed in the defaults and takes care if default values are changed or new fields are added in the future versions. However, I did not like the casting part in this. Nonetheless, it appears that the "Clone" function on "http.Transport" was added specifically to solve this problem (golang/go#26013).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, @manish-sethi . Since the beginning, the master didn't use the clone of DefaultTransport.

transport := &http.Transport{

When I made the changes in master, I appended the other parameters to the existing definition. I wasn't aware of this clone operation (thanks for pointing this out).

On a different note, http.DefaultTransport https://github.com/golang/go/blob/master/src/net/http/transport.go#L42 is a package level state right? Hence, I could defend by saying that we are against defining package-level states and also the usage of existing package-level states (on a funny note).

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.

3 participants