Skip to content

Conversation

@kschubert
Copy link

At Datameer, we have had failing communication between client and server when the client reused HTTP connections. When I looked at the code, I noticed that the author(s) of the ConnectionKeepAliveStrategy seemed to think that it does what is actually the purpose of a ConnectionReuseStrategy. The comments within getKeepAliveDuration(..) suggest that they thought that by returning -1 they disable connection reuse (it actually means "keep the connection alive indefinitely").
However, I'm not sure if that is the real root cause of our issues regarding failing communication between client and server when connections are reused, so I designed the new code so that connections are never reused unless told to via property.

At Datameer, we have had failing communication between client and server
when the client reused HTTP connections. When I looked at the code, I
noticed that the author(s) of the ConnectionKeepAliveStrategy seemed to
think that it does what is actually the purpose of a
ConnectionReuseStrategy. The comments within getKeepAliveDuration(..)
suggest that they thought that by returning -1 they disable connection
reuse (it actually means "keep the connection alive indefinitely").
However, I'm not sure if that is the real root cause of our issues
regarding failing communication between client and server when connections
are reused, so I designed the new code so that connections are never reused
unless told to via property.
@zhicwu
Copy link
Contributor

zhicwu commented Feb 10, 2021

Thanks @kschubert. To respect server keep-alive timeout setting, PR #515 was merged and released in 0.2.5. Connection setting keepAliveTimeout is deprecated and will be removed in 0.3.0. What's the issue you had? Did you find disabling connection reuse helpful? If the issue is "host failed to respond", I'm afraid we have to go with retry(as in #540).

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