Set defaults and expose configuration of tchannel timeouts. #2173
Conversation
|
Cleans up idle tchannel connections/goroutines seen in the following profiles.
|
Codecov Report
@@ Coverage Diff @@
## master #2173 +/- ##
========================================
Coverage ? 51.6%
========================================
Files ? 102
Lines ? 9047
Branches ? 0
========================================
Hits ? 4674
Misses ? 4013
Partials ? 360
Continue to review full report at Codecov.
|
| @@ -573,3 +576,9 @@ func IsSeedNode(initialCluster []environment.SeedNode, hostID string) bool { | |||
|
|
|||
| return false | |||
| } | |||
|
|
|||
| // TchannelConfiguration holds tchannel config options. | |||
There was a problem hiding this comment.
nit: TChannelConfiguration
| @@ -157,6 +157,9 @@ type DBConfiguration struct { | |||
| // Limits contains configuration for limits that can be applied to M3DB for the purposes | |||
| // of applying back-pressure or protecting the db nodes. | |||
| Limits Limits `yaml:"limits"` | |||
|
|
|||
| // Tchannel exposes tchannel config options. | |||
| Tchannel TchannelConfiguration `yaml:"tchannel"` | |||
There was a problem hiding this comment.
nit: TChannel
Would it also be better to make this a pointer to be able to more easily tell when it's not been set and avoid default values? Looks like 0s get filtered out later but may be an option?
src/dbnode/client/options.go
Outdated
| @@ -199,6 +199,12 @@ var ( | |||
| SetJitter(true), | |||
| ) | |||
|
|
|||
| // defaultChannelOptions are tchannel channel options defaults. | |||
There was a problem hiding this comment.
nit: defaultChannelOptions are default TChannel options.
src/dbnode/server/server.go
Outdated
| tchannelOpts.MaxIdleTime = cfg.Tchannel.MaxIdleTime | ||
| tchannelOpts.IdleCheckInterval = cfg.Tchannel.IdleCheckInterval |
There was a problem hiding this comment.
nit: Does this need to check that tchannelOpts.MaxIdleTime >= tchannelOpts.IdleCheckInterval?
There was a problem hiding this comment.
The check interval can be independent of the max idle time.
| tchannelOpts.MaxIdleTime = cfg.TChannel.MaxIdleTime | ||
| tchannelOpts.IdleCheckInterval = cfg.TChannel.IdleCheckInterval |
There was a problem hiding this comment.
Do these need to be greater than 0?
There was a problem hiding this comment.
There are checks in the tchannel code already.
func (o *ChannelOptions) validateIdleCheck() error {
if o.IdleCheckInterval > 0 && o.MaxIdleTime <= 0 {
return errMaxIdleTimeNotSet
}
return nil
}
func (is *idleSweep) start() {
if is.started || is.idleCheckInterval <= 0 {
return
}
is.ch.log.WithFields(
LogField{"idleCheckInterval", is.idleCheckInterval},
LogField{"maxIdleTime", is.maxIdleTime},
).Info("Starting idle connections poller.")
is.started = true
is.stopCh = make(chan struct{})
go is.pollerLoop()
}
| @@ -158,8 +158,8 @@ type DBConfiguration struct { | |||
| // of applying back-pressure or protecting the db nodes. | |||
| Limits Limits `yaml:"limits"` | |||
|
|
|||
| // Tchannel exposes tchannel config options. | |||
| Tchannel TchannelConfiguration `yaml:"tchannel"` | |||
| // TChannel exposes tchannel config options. | |||
There was a problem hiding this comment.
nit: "TChannel" in the comment
| @@ -577,8 +577,8 @@ func IsSeedNode(initialCluster []environment.SeedNode, hostID string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // TchannelConfiguration holds tchannel config options. | |||
| type TchannelConfiguration struct { | |||
| // TChannelConfiguration holds tchannel config options. | |||
There was a problem hiding this comment.
nit: "TChannel" in the comment
| @@ -199,6 +199,12 @@ var ( | |||
| SetJitter(true), | |||
| ) | |||
|
|
|||
| // defaultChannelOptions are default tchannel channel options. | |||
| defaultChannelOptions = &tchannel.ChannelOptions{ | |||
| MaxIdleTime: 5 * time.Minute, | |||
There was a problem hiding this comment.
nit: Probably would've opted for 1min max idle time and 1-2 minute idle check interval, just since 1min in this world is a very long time heh.
What this PR does / why we need it:
Set defaults and allow configuration of tchannel options
MaxIdleTimeoutandIdleCheckIntervalto allow cleaning up of idle tchannel connections.Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: