search

Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.61k stars 594 forks source link

Allow configuring timeToLive for the connection pool #286

Closed maxceban closed 5 years ago

maxceban commented 5 years ago

Is your feature request related to a problem? Please describe. We are having some issues with regards to connection pools when releasing a new version of the service called by the unirest client. This release process involves updating the DNS entry for the load balancer and the clients need to refresh the connections in order to start calling the new version of the service. We're looking into making it happen by leveraging the timeToLive functionality of the CPool used by the underlying apache client. However, currently it's really hard to propagate this value to the pool instance and by default, this ttl is infinite.

Describe the solution you'd like Exposing an additional configuration parameter (connectionTimeToLive) in the kong.unirest.Config that would be picked up when initializing the PoolingNHttpClientConnectionManager from the ApacheAsyncClient.

Describe alternatives you've considered I've considered providing a custom instance of AsyncClient, but that involves a lot of duplication of the code already taken care of by ApacheAsyncClient.

ryber commented 5 years ago

So just to be clear the TTL on the pool is " life span of persistent connections regardless of their expiration setting. No persistent connection will be re-used past its TTL value." ... it does not appear to be directly related to DNS TTL, so once you bump your DNS, or finish moving whatever it is your moving (I presume), you will want to bump this back up and re-init the config. Having a really short TTL on the connection pool will reduce overall performance.

ryber commented 5 years ago

2.3.11 is now in maven central and has a new connectionTTL(42, TimeUnit.MILLISECONDS) method on the config to set this.

maxceban commented 5 years ago

That's correct, we're looking into to a No persistent connection will be re-used past its TTL value type of solution. I know that this is not the best solution and the right thing would be for the load balancer to properly signal to the client that the connection should be terminated, but unfortunately we're dealing with a bug on the balancer that doesn't follow this rule properly... As such, our clients hold on to the connections to the old balancer pretty much forever as long as these connections are being used. As a workaround, we're looking into making the old balancer available during a transition period while allowing the clients to tear down the connections and establish new ones (to the new balancer). I understand the implication of short TTL, but we're looking to use a reasonable value like 5 min, so it shouldn't be too bad. Of course we should keep the default behaviour as-is, but it would be great if we could control this via configuration

ryber commented 5 years ago

NP, it's done and published,

maxceban commented 5 years ago

Awesome, thanks so much Ryan! Going to give it a try in prod next week!