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

Set defaults and expose configuration of tchannel timeouts. #2173

Merged
merged 8 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/cmd/services/m3dbnode/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// InitDefaultsAndValidate initializes all default values and validates the Configuration.
Expand Down Expand Up @@ -573,3 +576,9 @@ func IsSeedNode(initialCluster []environment.SeedNode, hostID string) bool {

return false
}

// TchannelConfiguration holds tchannel config options.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: TChannelConfiguration

type TchannelConfiguration struct {
MaxIdleTime time.Duration `yaml:"maxIdleTime"`
IdleCheckInterval time.Duration `yaml:"idleCheckInterval"`
}
2 changes: 0 additions & 2 deletions src/dbnode/client/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"github.com/m3db/m3/src/dbnode/environment"
"github.com/m3db/m3/src/dbnode/namespace"
"github.com/m3db/m3/src/dbnode/topology"
xtchannel "github.com/m3db/m3/src/dbnode/x/tchannel"
xerrors "github.com/m3db/m3/src/x/errors"
"github.com/m3db/m3/src/x/ident"
"github.com/m3db/m3/src/x/instrument"
Expand Down Expand Up @@ -297,7 +296,6 @@ func (c Configuration) NewAdminClient(
v := NewAdminOptions().
SetTopologyInitializer(syncTopoInit).
SetAsyncTopologyInitializers(asyncTopoInits).
SetChannelOptions(xtchannel.NewDefaultChannelOptions()).
SetInstrumentOptions(iopts)

if c.UseV2BatchAPIs != nil {
Expand Down
7 changes: 7 additions & 0 deletions src/dbnode/client/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,12 @@ var (
SetJitter(true),
)

// defaultChannelOptions are tchannel channel options defaults.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: defaultChannelOptions are default TChannel options.

defaultChannelOptions = &tchannel.ChannelOptions{
MaxIdleTime: 5 * time.Minute,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

IdleCheckInterval: 5 * time.Minute,
}

errNoTopologyInitializerSet = errors.New("no topology initializer set")
errNoReaderIteratorAllocateSet = errors.New("no reader iterator allocator set, encoding not set")
)
Expand Down Expand Up @@ -312,6 +318,7 @@ func newOptions() *options {
opts := &options{
clockOpts: clock.NewOptions(),
instrumentOpts: instrument.NewOptions(),
channelOptions: defaultChannelOptions,
writeConsistencyLevel: defaultWriteConsistencyLevel,
readConsistencyLevel: defaultReadConsistencyLevel,
bootstrapConsistencyLevel: defaultBootstrapConsistencyLevel,
Expand Down
13 changes: 11 additions & 2 deletions src/dbnode/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ import (
apachethrift "github.com/apache/thrift/lib/go/thrift"
opentracing "github.com/opentracing/opentracing-go"
"github.com/uber-go/tally"
"github.com/uber/tchannel-go"
"go.etcd.io/etcd/embed"
"go.uber.org/zap"
)
Expand Down Expand Up @@ -599,6 +600,10 @@ func Run(runOpts RunOptions) {
// SetDatabase() once we've initialized it.
service = ttnode.NewService(nil, ttopts)
)
if cfg.Tchannel.MaxIdleTime > 0 && cfg.Tchannel.IdleCheckInterval > 0 {
tchannelOpts.MaxIdleTime = cfg.Tchannel.MaxIdleTime
tchannelOpts.IdleCheckInterval = cfg.Tchannel.IdleCheckInterval
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Does this need to check that tchannelOpts.MaxIdleTime >= tchannelOpts.IdleCheckInterval?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check interval can be independent of the max idle time.

}
tchannelthriftNodeClose, err := ttnode.NewServer(service,
cfg.ListenAddress, contextPool, tchannelOpts).ListenAndServe()
if err != nil {
Expand Down Expand Up @@ -694,7 +699,7 @@ func Run(runOpts RunOptions) {

origin := topology.NewHost(hostID, "")
m3dbClient, err := newAdminClient(
cfg.Client, iopts, syncCfg.TopologyInitializer, runtimeOptsMgr,
cfg.Client, iopts, tchannelOpts, syncCfg.TopologyInitializer, runtimeOptsMgr,
origin, protoEnabled, schemaRegistry, syncCfg.KVStore, logger)
if err != nil {
logger.Fatal("could not create m3db client", zap.Error(err))
Expand Down Expand Up @@ -729,7 +734,7 @@ func Run(runOpts RunOptions) {
// Guaranteed to not be nil if repair is enabled by config validation.
clientCfg := *cluster.Client
clusterClient, err := newAdminClient(
clientCfg, iopts, topologyInitializer, runtimeOptsMgr,
clientCfg, iopts, tchannelOpts, topologyInitializer, runtimeOptsMgr,
origin, protoEnabled, schemaRegistry, syncCfg.KVStore, logger)
if err != nil {
logger.Fatal(
Expand Down Expand Up @@ -1544,6 +1549,7 @@ func withEncodingAndPoolingOptions(
func newAdminClient(
config client.Configuration,
iopts instrument.Options,
tchannelOpts *tchannel.ChannelOptions,
topologyInitializer topology.Initializer,
runtimeOptsMgr m3dbruntime.OptionsManager,
origin topology.Host,
Expand All @@ -1564,6 +1570,9 @@ func newAdminClient(
SetMetricsScope(iopts.MetricsScope().SubScope("m3dbclient")),
TopologyInitializer: topologyInitializer,
},
func(opts client.AdminOptions) client.AdminOptions {
return opts.SetChannelOptions(tchannelOpts).(client.AdminOptions)
},
func(opts client.AdminOptions) client.AdminOptions {
return opts.SetRuntimeOptionsManager(runtimeOptsMgr).(client.AdminOptions)
},
Expand Down
15 changes: 13 additions & 2 deletions src/dbnode/x/tchannel/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,22 @@

package xtchannel

import tchannel "github.com/uber/tchannel-go"
import (
"time"

tchannel "github.com/uber/tchannel-go"
)

const (
defaultIdleCheckInterval = 5 * time.Minute
defaultMaxIdleTime = 5 * time.Minute
)

// NewDefaultChannelOptions returns the default tchannel options used.
func NewDefaultChannelOptions() *tchannel.ChannelOptions {
return &tchannel.ChannelOptions{
Logger: NewNoopLogger(),
Logger: NewNoopLogger(),
MaxIdleTime: defaultMaxIdleTime,
IdleCheckInterval: defaultIdleCheckInterval,
}
}