-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add ability to set HttpClient Connection TTL #1893
Comments
@meltsufin you want to use A couple of notes:
|
@sbordet What we're really looking for is non-idle timeout. Interestingly, the "hot" connections is bad for auto-scaling/rebalancing for us. Is there a way to change this behavior? |
What do you mean by non-idle timeout ? If it's not idle why you want it to timeout ? Regarding "hot" connections, yes, we have implemented an alternative but it's currently private. |
See also #1897. |
Do you need a live timeout in time (e.g. 60 seconds), or in number of requests/connection (e.g. 500 requests at most) ? If it is measured in time, do you need the timeout to be respected precisely, or a best effort is enough ? For example, the implementation can enforce the live timeout on the connection pool release event; this however means that the connection may be used, then go idle, then the live timeout expire, then some time later be used again and when released the implementation will notice that the live timeout was expired, so it will close the connection - possibly quite later than the live timeout. |
Time-based expiration is good, and I think best effort is fine. We wouldn't want to impact the HttpClient performance negatively with this setting or require extra threads to enforce the setting precisely. Thanks! |
@sbordet One problem we're seeing now is that all connections in the pool close and re-open at the same time. Is there a way to add some randomness to the TTL? |
@meltsufin randomness can be added, but then again all connections will be closed within the random interval, which may be longer or comparable with the time to live, at which point will become less meaningful (i.e. set a time to live, but then it lasts twice as much, or zero). Adding randomness would be trivial, but I feel I'm missing the real reason for all this and maybe there is a better way to address it ? |
@meltsufin I assume that the problem is not so much the "close at the same time", but the "re-open at the same time", which would be additional load on the systems? Also by "all" do you mean actually "all", or just "most", "many", "more than one"? I'm guessing the size of the close events will be related to bursts of activity? So that if 10 connections were used in one burst of activity, then all 10 will timeout together. We could add randomness in the timeout, but then as we only close such a connection after it has been re-used, the time of the re-use is itself moderately random.... unless there is a burst of usage again. Perhaps a reuse count would be better than a reuse timeout, as that would be unaffected by idle periods... ie an idle client would not collect TTL connections that will all be closed when next used. |
@sbordet @gregw Yes, the problem is the re-open at the same time putting a lot of pressure on the system. The activity is more or less constant. Imagine 1000 connections all loaded and reconnecting every 60 seconds. You get a burst of reconnects every 60 seconds. If I understand correctly, re-use count may be more random if the request latency has some variance. I'm willing to try it. Another solution might be to just let the user specify a TTL on a per-connection basis using an exposed strategy. WDYT? |
@meltsufin I like the idea of adding a |
@sbordet It's the old solution we had. We're still testing it out in combination with round-robin. At this time |
Removed the functionality, not needed anymore.
Removed test for TTL functionality. Added test for the round robin behavior.
Jetty HttpClient pools HTTP connections and we need an ability to set a TTL on the connections in the pool so that they're eventually closed an re-established. Is there a way to do this?
@gregw @olamy
The text was updated successfully, but these errors were encountered: