improvements such as backward dc client config compatibility, default…

… config store client values, better adherence to conventions, etc.

.gitmodules

@@ -1,4 +1,4 @@
 [submodule "idls"]
 	path = idls
 	url = https://github.com/uber/cadence-idl.git
-	branch = configurationStore
+	branch = master

client/admin/client.go

@@ -384,7 +384,7 @@ func (c *clientImpl) GetDynamicConfig(
 	if err != nil {
 		return nil, err
 	}
-	ctx, cancel := c.createContextWithLargeTimeout(ctx)
+	ctx, cancel := c.createContext(ctx)
 	defer cancel()
 	return client.GetDynamicConfig(ctx, request, opts...)
 }
@@ -399,7 +399,7 @@ func (c *clientImpl) UpdateDynamicConfig(
 	if err != nil {
 		return err
 	}
-	ctx, cancel := c.createContextWithLargeTimeout(ctx)
+	ctx, cancel := c.createContext(ctx)
 	defer cancel()
 	return client.UpdateDynamicConfig(ctx, request, opts...)
 }
@@ -414,7 +414,7 @@ func (c *clientImpl) RestoreDynamicConfig(
 	if err != nil {
 		return err
 	}
-	ctx, cancel := c.createContextWithLargeTimeout(ctx)
+	ctx, cancel := c.createContext(ctx)
 	defer cancel()
 	return client.RestoreDynamicConfig(ctx, request, opts...)
 }
@@ -429,7 +429,7 @@ func (c *clientImpl) ListDynamicConfig(
 	if err != nil {
 		return nil, err
 	}
-	ctx, cancel := c.createContextWithLargeTimeout(ctx)
+	ctx, cancel := c.createContext(ctx)
 	defer cancel()
 	return client.ListDynamicConfig(ctx, request, opts...)
 }

cmd/server/cadence/cadence.go

@@ -56,7 +56,11 @@ func startHandler(c *cli.Context) {
 	if cfg.Log.Level == "debug" {
 		log.Printf("config=\n%v\n", cfg.String())
 	}
-	cfg.DynamicConfig.FileBased.Filepath = constructPathIfNeed(rootDir, cfg.DynamicConfig.FileBased.Filepath)
+	if cfg.DynamicConfig.Client == "" {
+		cfg.DynamicConfigClient.Filepath = constructPathIfNeed(rootDir, cfg.DynamicConfigClient.Filepath)
+	} else {
+		cfg.DynamicConfig.FileBased.Filepath = constructPathIfNeed(rootDir, cfg.DynamicConfig.FileBased.Filepath)
+	}
 
 	if err := cfg.ValidateAndFillDefaults(); err != nil {
 		log.Fatalf("config validation failed: %v", err)

cmd/server/cadence/server.go

@@ -114,25 +114,31 @@ func (s *server) startService() common.Daemon {
 	params.UpdateLoggerWithServiceName(params.Name)
 	params.PersistenceConfig = s.cfg.Persistence
 
-	switch s.cfg.DynamicConfig.Client {
-	case dynamicconfig.DynamicConfigConfigStoreClient:
-		params.DynamicConfig, err = configstore.NewConfigStoreClient(
-			&s.cfg.DynamicConfig.ConfigStore,
-			s.cfg.Persistence.DataStores[s.cfg.Persistence.DefaultStore].NoSQL,
-			params.Logger,
-			s.doneC,
-		)
-		log.Printf("Initializing Config Store Dynamic Config Client\n")
-	case dynamicconfig.DynamicConfigFileBasedClient:
-		params.DynamicConfig, err = dynamicconfig.NewFileBasedClient(&s.cfg.DynamicConfig.FileBased, params.Logger, s.doneC)
-		log.Printf("Initializing File Based Dynamic Config Client\n")
-	default:
-		params.DynamicConfig = dynamicconfig.NewNopClient()
-		err = nil
+	err = nil
+	if s.cfg.DynamicConfig.Client == "" {
+		//try to fallback to legacy dynamicClientConfig
+		params.DynamicConfig, err = dynamicconfig.NewFileBasedClient(&s.cfg.DynamicConfigClient, params.Logger, s.doneC)
+	} else {
+		switch s.cfg.DynamicConfig.Client {
+		case dynamicconfig.DynamicConfigConfigStoreClient:
+			log.Printf("Trying to initialize Config Store Dynamic Config Client\n")
+			params.DynamicConfig, err = configstore.NewConfigStoreClient(
+				&s.cfg.DynamicConfig.ConfigStore,
+				&s.cfg.Persistence,
+				params.Logger,
+				s.doneC,
+			)
+		case dynamicconfig.DynamicConfigFileBasedClient:
+			log.Printf("Trying to initialize File Based Dynamic Config Client\n")
+			params.DynamicConfig, err = dynamicconfig.NewFileBasedClient(&s.cfg.DynamicConfig.FileBased, params.Logger, s.doneC)
+		default:
+			log.Printf("Trying to initialize Nop Config Client\n")
+			params.DynamicConfig = dynamicconfig.NewNopClient()
+		}
 	}
 
 	if err != nil {
-		log.Printf("error creating config store based dynamic config client, use no-op config client instead. error: %v", err)
+		log.Printf("error creating dynamic config client, using no-op config client instead. error: %v", err)
 		params.DynamicConfig = dynamicconfig.NewNopClient()
 	}
 	clusterMetadata := s.cfg.ClusterMetadata

common/config/config.go

@@ -32,7 +32,7 @@ import (
 
 	"github.com/uber/cadence/common"
 	"github.com/uber/cadence/common/dynamicconfig"
-	"github.com/uber/cadence/common/dynamicconfig/configstore/configstoreconfig"
+	c "github.com/uber/cadence/common/dynamicconfig/configstore/config"
 )
 
 type (
@@ -58,6 +58,10 @@ type (
 		PublicClient PublicClient `yaml:"publicClient"`
 		// DynamicConfigClient is the config for setting up the file based dynamic config client
 		// Filepath would be relative to the root directory when the path wasn't absolute.
+		// Included for backwards compatibility, please transition to DynamicConfig
+		DynamicConfigClient dynamicconfig.FileBasedClientConfig `yaml:"dynamicConfigClient"`
+		// DynamicConfig is the config for setting up all dynamic config clients
+		// Allows for changes in client without needing code change
 		DynamicConfig DynamicConfig `yaml:"dynamicconfig"`
 		// DomainDefaults is the default config for every domain
 		DomainDefaults DomainDefaults `yaml:"domainDefaults"`
@@ -73,9 +77,9 @@ type (
 	}
 
 	DynamicConfig struct {
-		Client      string                                    `yaml:"client"`
-		ConfigStore configstoreconfig.ConfigStoreClientConfig `yaml:"configstore"`
-		FileBased   dynamicconfig.FileBasedClientConfig       `yaml:"filebased"`
+		Client      string                              `yaml:"client"`
+		ConfigStore c.ClientConfig                      `yaml:"configstore"`
+		FileBased   dynamicconfig.FileBasedClientConfig `yaml:"filebased"`
 	}
 
 	NoopAuthorizer struct {

common/dynamicconfig/configstore/configstoreconfig/config.go → common/dynamicconfig/configstore/config/config.go

@@ -18,13 +18,16 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.
 
-package configstoreconfig
+package config
 
 import "time"
 
-// ConfigStoreClientConfig is the config for the config store based dynamic config client.
+//This package is necessary to avoid import cycle as config_store_client imports common/config
+//while common/config imports this ClientConfig definition
+
+// ClientConfig is the config for the config store based dynamic config client.
 // It specifies how often the cached config should be updated by checking underlying database.
-type ConfigStoreClientConfig struct {
+type ClientConfig struct {
 	PollInterval        time.Duration `yaml:"pollInterval"`
 	UpdateRetryAttempts int           `yaml:"updateRetryAttempts"`
 	FetchTimeout        time.Duration `yaml:"FetchTimeout"`

common/dynamicconfig/configstore/config_store_client.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2017 Uber Technologies, Inc.
+// Copyright (c) 2021 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -31,7 +31,7 @@ import (
 
 	"github.com/uber/cadence/common/config"
 	dc "github.com/uber/cadence/common/dynamicconfig"
-	csc "github.com/uber/cadence/common/dynamicconfig/configstore/configstoreconfig"
+	csc "github.com/uber/cadence/common/dynamicconfig/configstore/config"
 	"github.com/uber/cadence/common/log"
 	"github.com/uber/cadence/common/log/tag"
 	"github.com/uber/cadence/common/persistence"
@@ -45,10 +45,17 @@ const (
 	configStoreMinPollInterval = time.Second * 2
 )
 
+var defaultConfigValues = &csc.ClientConfig{
+	PollInterval:        time.Second * 10,
+	UpdateRetryAttempts: 1,
+	FetchTimeout:        2,
+	UpdateTimeout:       2,
+}
+
 type configStoreClient struct {
 	values             atomic.Value
 	lastUpdatedTime    time.Time
-	config             *csc.ConfigStoreClientConfig
+	config             *csc.ClientConfig
 	configStoreManager persistence.ConfigStoreManager
 	doneCh             chan struct{}
 	logger             log.Logger
@@ -66,8 +73,23 @@ type fetchResult struct {
 }
 
 // NewConfigStoreClient creates a config store client
-func NewConfigStoreClient(clientCfg *csc.ConfigStoreClientConfig, persistenceCfg *config.NoSQL, logger log.Logger, doneCh chan struct{}) (dc.Client, error) {
-	client, err := newConfigStoreClient(clientCfg, persistenceCfg, logger, doneCh)
+func NewConfigStoreClient(clientCfg *csc.ClientConfig, persistenceCfg *config.Persistence, logger log.Logger, doneCh chan struct{}) (dc.Client, error) {
+	if err := validateClientConfig(clientCfg); err != nil {
+		logger.Error("Invalid Client Config Values, Using Default Values")
+		clientCfg = defaultConfigValues
+	}
+
+	if persistenceCfg == nil {
+		return nil, errors.New("persistence cfg is nil")
+	} else if persistenceCfg.DefaultStore != "cass-default" {
+		return nil, errors.New("persistence cfg default store is not Cassandra")
+	} else if store, ok := persistenceCfg.DataStores[persistenceCfg.DefaultStore]; !ok {
+		return nil, errors.New("persistence cfg datastores missing Cassandra")
+	} else if store.NoSQL == nil {
+		return nil, errors.New("NoSQL struct is nil")
+	}
+
+	client, err := newConfigStoreClient(clientCfg, persistenceCfg.DataStores[persistenceCfg.DefaultStore].NoSQL, logger, doneCh)
 	if err != nil {
 		return nil, err
 	}
@@ -78,11 +100,7 @@ func NewConfigStoreClient(clientCfg *csc.ConfigStoreClientConfig, persistenceCfg
 	return client, nil
 }
 
-func newConfigStoreClient(clientCfg *csc.ConfigStoreClientConfig, persistenceCfg *config.NoSQL, logger log.Logger, doneCh chan struct{}) (*configStoreClient, error) {
-	if err := validateConfigStoreClientConfig(clientCfg); err != nil {
-		return nil, err
-	}
-
+func newConfigStoreClient(clientCfg *csc.ClientConfig, persistenceCfg *config.NoSQL, logger log.Logger, doneCh chan struct{}) (*configStoreClient, error) {
 	store, err := nosql.NewNoSQLConfigStore(*persistenceCfg, logger)
 	if err != nil {
 		return nil, err
@@ -205,20 +223,20 @@ func (csc *configStoreClient) GetDurationValue(
 		return defaultValue, err
 	}
 
-	durationString, ok := val.(string)
-	if !ok {
-		durVal, ok2 := val.(time.Duration)
-		if ok2 {
-			return durVal, nil
+	var durVal time.Duration
+	switch v := val.(type) {
+	case string:
+		durVal, err = time.ParseDuration(v)
+		if err != nil {
+			return defaultValue, errors.New("value string encoding cannot be parsed into duration")
 		}
-		return defaultValue, errors.New("value type is not string")
+	case time.Duration:
+		durVal = v
+	default:
+		return defaultValue, errors.New("value type is not duration")
 	}
 
-	durationVal, err := time.ParseDuration(durationString)
-	if err != nil {
-		return defaultValue, fmt.Errorf("failed to parse duration: %v", err)
-	}
-	return durationVal, nil
+	return durVal, nil
 }
 
 func (csc *configStoreClient) UpdateValue(name dc.Key, value interface{}) error {
@@ -358,18 +376,21 @@ func (csc *configStoreClient) updateValue(name dc.Key, value interface{}, retryA
 		},
 	}
 
-	updateCh := make(chan error)
-	go func() {
-		updateCh <- csc.configStoreManager.UpdateDynamicConfig(
-			context.TODO(),
-			&persistence.UpdateDynamicConfigRequest{
-				Snapshot: newSnapshot,
-			},
-		)
-	}()
+	ctx, cancel := context.WithTimeout(context.Background(), csc.config.UpdateTimeout)
+	defer cancel()
+
+	err := csc.configStoreManager.UpdateDynamicConfig(
+		ctx,
+		&persistence.UpdateDynamicConfigRequest{
+			Snapshot: newSnapshot,
+		},
+	)
 
 	select {
-	case err := <-updateCh:
+	case <-ctx.Done():
+		//potentially we can retry on timeout
+		return errors.New("timeout error on update")
+	default:
 		if err != nil {
 			if _, ok := err.(*persistence.ConditionFailedError); ok && retryAttempts > 0 {
 				//fetch new config and retry
@@ -386,9 +407,6 @@ func (csc *configStoreClient) updateValue(name dc.Key, value interface{}, retryA
 			return err
 		}
 		return nil
-	case <-time.After(csc.config.UpdateTimeout):
-		return errors.New("timeout error on update")
-		//should we retry on timeout errors
 	}
 }
 
@@ -455,34 +473,28 @@ func copyDataBlob(blob *types.DataBlob) *types.DataBlob {
 }
 
 func (csc *configStoreClient) update() error {
-	fetchCh := make(chan *fetchResult)
-	go func() {
-		res, err := csc.configStoreManager.FetchDynamicConfig(context.TODO())
-		if res == nil {
-			fetchCh <- &fetchResult{snapshot: nil, err: err}
-		} else {
-			fetchCh <- &fetchResult{snapshot: res.Snapshot, err: err}
-		}
-	}()
+	ctx, cancel := context.WithTimeout(context.Background(), csc.config.FetchTimeout)
+	defer cancel()
+
+	res, err := csc.configStoreManager.FetchDynamicConfig(ctx)
 
 	select {
-	case fetchRes := <-fetchCh:
-		if fetchRes.err != nil {
-			return fmt.Errorf("failed to fetch dynamic config snapshot %v", fetchRes.err)
+	case <-ctx.Done():
+		return errors.New("timeout error on fetch")
+	default:
+		if err != nil {
+			return fmt.Errorf("failed to fetch dynamic config snapshot %v", err)
 		}
 
-		if fetchRes.snapshot != nil {
+		if res != nil && res.Snapshot != nil {
 			defer func() {
 				csc.lastUpdatedTime = time.Now()
 			}()
 
-			return csc.storeValues(fetchRes.snapshot)
+			return csc.storeValues(res.Snapshot)
 		}
-
-		return nil
-	case <-time.After(csc.config.FetchTimeout):
-		return errors.New("timeout error on fetch")
 	}
+	return nil
 }
 
 func (csc *configStoreClient) storeValues(snapshot *persistence.DynamicConfigSnapshot) error {
@@ -557,7 +569,7 @@ func matchFilters(dcValue *types.DynamicConfigValue, filters map[dc.Filter]inter
 	return true
 }
 
-func validateConfigStoreClientConfig(config *csc.ConfigStoreClientConfig) error {
+func validateClientConfig(config *csc.ClientConfig) error {
 	if config == nil {
 		return errors.New("no config found for config store based dynamic config client")
 	}