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

Introduce ClientSession configuration section that provides ways to control idleTime and lifeTime of client connections. #1903

Merged

Conversation

Projects
None yet
4 participants
@nikolay-pshenichny
Copy link
Contributor

commented Apr 18, 2018

This PR introduces a new client configuration section that allows to control idleTime and lifeTime of the client connections.

Fixes #1890

@adleong
Copy link
Member

left a comment

Very nice work, @nikolay-pshenichny, this is great!

Could you please amend your commit with the DCO signoff so that we can accept this PR?

Introduce ClientSession configuration section that provides ways to c…
…ontrol idleTime and lifeTime of client connections.

Signed-off-by: Nikolay Pshenichnyy <nikolay.pshenichny@gmail.com>

@nikolay-pshenichny nikolay-pshenichny force-pushed the nikolay-pshenichny:npshenichnyy/connection-lifetime branch from ac0e60e to 30a36d2 Apr 18, 2018

@nikolay-pshenichny

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@adleong , done.

@@ -269,5 +273,14 @@ Key | Default Value | Description
--- | ------------- | -----------
minSize | `0` | The minimum number of connections to maintain to each host.
maxSize | Int.MaxValue | The maximum number of connections to maintain to each host.
idleTimeMs | forever | The amount of idle time for which a connection is cached in milliseconds.
idleTimeMs | forever | The amount of idle time for which a connection is cached in milliseconds. Only applied to connections that number greater than minSize, but fewer than maxSize.

This comment has been minimized.

Copy link
@dadjeibaah

dadjeibaah Apr 19, 2018

Member

I am trying to understand this comment. How do HostConnectionPool's idleTimeMs differ from ClientSession's idleTimeMs? Do they effectively do the same thing? If they do, then we may no longer need to have this setting in HostConnectionPool.

This comment has been minimized.

Copy link
@adleong

adleong Apr 19, 2018

Member

They are similar but slightly different. This connection timeout is only applied to connections between the minSize and maxSize whereas the clientSessions timeouts apply to all connections. The connectionPool timeout can be thought of as the amount of time "extra" connections are kept around.

This comment has been minimized.

Copy link
@adleong

adleong Apr 19, 2018

Member

Although, in the case where minSize=0 (which is the default) then these two timeouts do exactly the same thing.

This comment has been minimized.

Copy link
@dadjeibaah

dadjeibaah Apr 19, 2018

Member

Ahh I see, thanks for the clarification!

}


}

This comment has been minimized.

Copy link
@dadjeibaah

dadjeibaah Apr 19, 2018

Member

I may be wrong, but maybe we could add a test that exercises both hostConnectionPool and clientSession configs. I am still unclear of how the two different kinds of configurations would work together if that ever is the case.

This comment has been minimized.

Copy link
@nikolay-pshenichny

nikolay-pshenichny Apr 19, 2018

Author Contributor

@dadjeibaah , I can look into adding more tests for the scenarios when both configurations are used. But I will be able to do that only starting Tuesday (24th) next week.

@adleong

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@nikolay-pshenichny Looks like there's a merge conflict with the latest master. Would you mind resolving that and then we should be good to merge.

@nikolay-pshenichny

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@adleong , I will look into that a little later today.

Merge branch 'master' into npshenichnyy/connection-lifetime and resol…
…ve conflicts

Signed-off-by: Nikolay Pshenichnyy <nikolay.pshenichny@gmail.com>
@nikolay-pshenichny

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@adleong , resolved

@adleong

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

⭐️ Thank you so much for tracking this down and fixing it, @nikolay-pshenichny!

@adleong adleong merged commit 9542e3e into linkerd:master Apr 19, 2018

2 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci Your tests passed on CircleCI!
Details
@wmorgan

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

@nikolay-pshenichny nice work!

mmrozek added a commit to mmrozek/linkerd that referenced this pull request Apr 20, 2018

Introduce ClientSession configuration section that provides ways to c…
…ontrol idleTime and lifeTime of client connections. (linkerd#1903)

This PR introduces a new client configuration section that allows to control `idleTime` and `lifeTime` of the client connections.

Fixes linkerd#1890

Signed-off-by: Nikolay Pshenichnyy <nikolay.pshenichny@gmail.com>

@adleong adleong referenced this pull request Apr 30, 2018

Merged

1.4.0 #1924

adleong added a commit that referenced this pull request Apr 30, 2018

1.4.0 (#1924)
Linkerd 1.4.0 upgrades us to the latest versions of Finagle and Netty and
features lower memory usage for large payloads. Two new configuration options
have been introduced: client connection lifetimes and access log rotation
policy. One breaking change has been introduced around the configuration file
syntax for loggers.  This release features contributions from ThreeComma,
[ScalaConsultants](https://github.com/ScalaConsultants), [Salesforce](https://github.com/salesforce), and [Buoyant](https://github.com/buoyantio).

* **Breaking Change**: Rename the loggers section of the Linkerd config to requestAuthorizers to match the name of the plugin type ([#1900](#1900))
* Tune Netty/Finagle settings to reduce direct memory usage ([#1889](#1889)). This should dramatically reduce direct memory usage when transferring large payloads.
* Introduce a ClientSession configuration section that provides ways to control client connection lifetime ([#1903](#1903)).
* Expose rotation policy configuration for http and http2 access logs ([#1893](#1893)).
* Stop logging harmless reader discarded errors in k8s namer ([#1901](#1901)).
* Disable autoloading of the default tracer in Namerd ([#1902](#1902)). This prevents Namerd from attempting to connect to a Zipkin collector that doesn't exist.
* Upgrade to Finagle 18.4.0.

Signed-off-by: Alex Leong <alex@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.