From 19a34aa540971d2bb7f5bba2017ce98039b37d25 Mon Sep 17 00:00:00 2001 From: Wei Han Date: Tue, 14 Mar 2017 14:05:23 -0700 Subject: [PATCH 1/2] allow customer to set only part of client option --- client/cherami/client.go | 50 ++++++++++++++++++++++++++++++++-------- common/util.go | 9 ++++---- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/client/cherami/client.go b/client/cherami/client.go index 270d073..29f8f05 100644 --- a/client/cherami/client.go +++ b/client/cherami/client.go @@ -85,10 +85,29 @@ func NewHyperbahnClient(serviceName string, bootstrapFile string, options *Clien return newClientWithTChannel(ch, options) } -// NewClientWithFE is used by Frontend to create a Cherami client for itself. -// It is used by non-streaming publish/consume APIs. +// NewClientWithFEClient is used by Cherami Frontend to create a Cherami client for itself. +// Cherami customers shouldn't need to use this function. +func NewClientWithFEClient(feClient cherami.TChanBFrontend, options *ClientOptions) (Client, error) { + options, err := verifyOptions(options) + if err != nil { + return nil, err + } + + return &clientImpl{ + client: feClient, + options: options, + retryPolicy: createDefaultRetryPolicy(), + }, nil +} + +// NewClientWithFEClient is used by Cherami Frontend to create a Cherami client for itself. +// Cherami customers shouldn't need to use this function. +// This function is deprecated, use NewClientWithFEClient instead func NewClientWithFE(feClient cherami.TChanBFrontend, options *ClientOptions) Client { - options = verifyOptions(options) + options, err := verifyOptions(options) + if err != nil { + panic(fmt.Sprintf(`Client option is invalid (and NewClientWithFEn is deprecated, use NewClientWithFEClient instead). Error: %v`, err)) + } return &clientImpl{ client: feClient, @@ -98,7 +117,10 @@ func NewClientWithFE(feClient cherami.TChanBFrontend, options *ClientOptions) Cl } func newClientWithTChannel(ch *tchannel.Channel, options *ClientOptions) (Client, error) { - options = verifyOptions(options) + options, err := verifyOptions(options) + if err != nil { + return nil, err + } tClient := thrift.NewClient(ch, getFrontEndServiceName(options.DeploymentStr), nil) @@ -255,7 +277,10 @@ func (c *clientImpl) CreatePublisher(request *CreatePublisherRequest) Publisher func (c *clientImpl) CreateConsumer(request *CreateConsumerRequest) Consumer { if request.Options != nil { - request.Options = verifyOptions(request.Options) + var err error + if request.Options, err = verifyOptions(request.Options); err != nil { + panic(fmt.Sprintf(`Client option is invalid (and CreateConsumerRequest.Options is deprecated). Error: %v`, err)) + } } else { request.Options = c.options } @@ -344,11 +369,16 @@ func getDefaultOptions() *ClientOptions { // verifyOptions is used to verify if we have a metrics reporter and // a logger. If not, just setup a default logger and a null reporter // it also validate the timeout is sane -func verifyOptions(opts *ClientOptions) *ClientOptions{ +func verifyOptions(opts *ClientOptions) (*ClientOptions, error){ if opts == nil { opts = getDefaultOptions() } - common.ValidateTimeout(opts.Timeout) + if opts.Timeout.Nanoseconds() == 0 { + opts.Timeout = getDefaultOptions().Timeout + } + if err := common.ValidateTimeout(opts.Timeout); err != nil { + return nil, err + } if opts.Logger == nil { opts.Logger = getDefaultLogger() @@ -357,14 +387,14 @@ func verifyOptions(opts *ClientOptions) *ClientOptions{ if opts.MetricsReporter == nil { opts.MetricsReporter = metrics.NewNullReporter() } + // Now make sure we init the default metrics as well + opts.MetricsReporter.InitMetrics(metrics.MetricDefs) if int64(opts.ReconfigurationPollingInterval/time.Second) == 0 { opts.ReconfigurationPollingInterval = defaultReconfigurationPollingInterval } - // Now make sure we init the default metrics as well - opts.MetricsReporter.InitMetrics(metrics.MetricDefs) - return opts + return opts, nil } func createDefaultRetryPolicy() backoff.RetryPolicy { diff --git a/common/util.go b/common/util.go index 5878b2c..390628d 100644 --- a/common/util.go +++ b/common/util.go @@ -24,6 +24,7 @@ import ( "encoding/binary" "encoding/hex" "encoding/json" + "errors" "fmt" "io/ioutil" "math/rand" @@ -276,13 +277,13 @@ func (avg *GeometricRollingAverage) GetGeometricRollingAverage() float64 { return float64(*avg) } -// ValidateTimeout panics if the passed timeout is unreasonable -func ValidateTimeout(t time.Duration) { +// ValidateTimeout returns an error if the passed timeout is unreasonable +func ValidateTimeout(t time.Duration) error { if t >= time.Millisecond*100 && t <= time.Minute*5 { - return + return nil } - panic(fmt.Sprintf(`Configured timeout is out of range: %v`, t)) + return errors.New(fmt.Sprintf(`Configured timeout is out of range: %v`, t)) } // MinInt returns the minimum of values (a, b) From fe270a7b5f4bf443d56dde7571a809e3d5176d4b Mon Sep 17 00:00:00 2001 From: Wei Han Date: Fri, 17 Mar 2017 10:06:15 -0700 Subject: [PATCH 2/2] address comments --- client/cherami/client.go | 18 ++---------------- common/util.go | 2 +- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/client/cherami/client.go b/client/cherami/client.go index 29f8f05..4cb033b 100644 --- a/client/cherami/client.go +++ b/client/cherami/client.go @@ -100,22 +100,6 @@ func NewClientWithFEClient(feClient cherami.TChanBFrontend, options *ClientOptio }, nil } -// NewClientWithFEClient is used by Cherami Frontend to create a Cherami client for itself. -// Cherami customers shouldn't need to use this function. -// This function is deprecated, use NewClientWithFEClient instead -func NewClientWithFE(feClient cherami.TChanBFrontend, options *ClientOptions) Client { - options, err := verifyOptions(options) - if err != nil { - panic(fmt.Sprintf(`Client option is invalid (and NewClientWithFEn is deprecated, use NewClientWithFEClient instead). Error: %v`, err)) - } - - return &clientImpl{ - client: feClient, - options: options, - retryPolicy: createDefaultRetryPolicy(), - } -} - func newClientWithTChannel(ch *tchannel.Channel, options *ClientOptions) (Client, error) { options, err := verifyOptions(options) if err != nil { @@ -277,6 +261,8 @@ func (c *clientImpl) CreatePublisher(request *CreatePublisherRequest) Publisher func (c *clientImpl) CreateConsumer(request *CreateConsumerRequest) Consumer { if request.Options != nil { + c.options.Logger.Warn(`CreateConsumerRequest.Options is deprecated. Client options should be set when creating the client`) + var err error if request.Options, err = verifyOptions(request.Options); err != nil { panic(fmt.Sprintf(`Client option is invalid (and CreateConsumerRequest.Options is deprecated). Error: %v`, err)) diff --git a/common/util.go b/common/util.go index 390628d..4f28b89 100644 --- a/common/util.go +++ b/common/util.go @@ -283,7 +283,7 @@ func ValidateTimeout(t time.Duration) error { return nil } - return errors.New(fmt.Sprintf(`Configured timeout is out of range: %v`, t)) + return errors.New(fmt.Sprintf(`Configured timeout [%v] must be in range [100ms-5min]`, t)) } // MinInt returns the minimum of values (a, b)