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 all 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"`
}

// 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.
type TChannelConfiguration struct {
MaxIdleTime time.Duration `yaml:"maxIdleTime"`
IdleCheckInterval time.Duration `yaml:"idleCheckInterval"`
}
1 change: 1 addition & 0 deletions src/cmd/services/m3dbnode/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ func TestConfiguration(t *testing.T) {
maxOutstandingWriteRequests: 0
maxOutstandingReadRequests: 0
maxOutstandingRepairedBytes: 0
tchannel: null
coordinator: null
`

Expand Down
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 default tchannel channel 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 != nil {
tchannelOpts.MaxIdleTime = cfg.TChannel.MaxIdleTime
tchannelOpts.IdleCheckInterval = cfg.TChannel.IdleCheckInterval
Comment on lines +604 to +605
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need to be greater than 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()
}

}
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,
}
}
21 changes: 18 additions & 3 deletions src/query/storage/m3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,21 @@ import (
"github.com/m3db/m3/src/query/stores/m3db"
"github.com/m3db/m3/src/x/ident"
"github.com/m3db/m3/src/x/instrument"

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

var (
errNotAggregatedClusterNamespace = goerrors.New("not an aggregated cluster namespace")
errBothNamespaceTypeNewAndDeprecatedFieldsSet = goerrors.New("cannot specify both deprecated and non-deprecated fields for namespace type")
)

// TODO(bodu): Could make these configurable at some point.
const (
idleCheckInterval = 5 * time.Minute
maxIdleTime = 5 * time.Minute
)

// ClustersStaticConfiguration is a set of static cluster configurations.
type ClustersStaticConfiguration []ClusterStaticConfiguration

Expand Down Expand Up @@ -187,9 +195,16 @@ func (c ClustersStaticConfiguration) NewClusters(

if opts.ProvidedSession == nil {
// NB(r): Only create client session if not already provided.
result, err = clusterCfg.newClient(client.ConfigurationParameters{
InstrumentOptions: instrumentOpts,
})
result, err = clusterCfg.newClient(
client.ConfigurationParameters{
InstrumentOptions: instrumentOpts,
},
func(opts client.Options) client.Options {
return opts.SetChannelOptions(&tchannel.ChannelOptions{
IdleCheckInterval: idleCheckInterval,
MaxIdleTime: maxIdleTime,
})
})
if err != nil {
return nil, err
}
Expand Down