From 93937370e177b0c12dc2df93e75f42d1e9e8b13d Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Thu, 2 Sep 2021 17:33:15 -0700 Subject: [PATCH] Refactor config methods for internal use (#4448) - Export config validate() and fillDefaults() methods for internal use - Move logic that will change persistence config values to pesistenceConfig.FillDefaults() --- common/config/cluster.go | 6 ++-- common/config/cluster_test.go | 4 +-- common/config/config.go | 20 ++----------- common/config/persistence.go | 53 ++++++++++++++++++++++++++--------- 4 files changed, 48 insertions(+), 35 deletions(-) diff --git a/common/config/cluster.go b/common/config/cluster.go index 1d00d624996..79729609870 100644 --- a/common/config/cluster.go +++ b/common/config/cluster.go @@ -80,7 +80,8 @@ type ( } ) -func (m *ClusterGroupMetadata) validate() error { +// Validate validates ClusterGroupMetadata +func (m *ClusterGroupMetadata) Validate() error { if m == nil { return errors.New("ClusterGroupMetadata cannot be empty") } @@ -142,7 +143,8 @@ func (m *ClusterGroupMetadata) validate() error { return errs } -func (m *ClusterGroupMetadata) fillDefaults() { +// FillDefaults populates default values for unspecified fields +func (m *ClusterGroupMetadata) FillDefaults() { if m == nil { return } diff --git a/common/config/cluster_test.go b/common/config/cluster_test.go index 2cddadf8681..5ef99004714 100644 --- a/common/config/cluster_test.go +++ b/common/config/cluster_test.go @@ -34,7 +34,7 @@ func TestClusterGroupMetadataDefaults(t *testing.T) { }, } - config.fillDefaults() + config.FillDefaults() assert.Equal(t, "active", config.PrimaryClusterName) assert.Equal(t, "cadence-frontend", config.ClusterGroup["active"].RPCName) @@ -164,7 +164,7 @@ func TestClusterGroupMetadataValidate(t *testing.T) { for _, tt := range tests { t.Run(tt.msg, func(t *testing.T) { - err := tt.config.validate() + err := tt.config.Validate() if tt.err == "" { assert.NoError(t, err) } else { diff --git a/common/config/config.go b/common/config/config.go index 06d4c8f95b1..531e9cd0c80 100644 --- a/common/config/config.go +++ b/common/config/config.go @@ -29,7 +29,6 @@ import ( "github.com/uber-go/tally/prometheus" "github.com/uber/ringpop-go/discovery" - "github.com/uber/cadence/common" "github.com/uber/cadence/common/dynamicconfig" c "github.com/uber/cadence/common/dynamicconfig/configstore/config" ) @@ -445,7 +444,7 @@ func (c *Config) validate() error { if err := c.Persistence.Validate(); err != nil { return err } - if err := c.ClusterGroupMetadata.validate(); err != nil { + if err := c.ClusterGroupMetadata.Validate(); err != nil { return err } if err := c.Archival.Validate(&c.DomainDefaults.Archival); err != nil { @@ -456,20 +455,7 @@ func (c *Config) validate() error { } func (c *Config) fillDefaults() { - // filling default encodingType/decodingTypes for SQL persistence - for k, store := range c.Persistence.DataStores { - if store.SQL != nil { - if store.SQL.EncodingType == "" { - store.SQL.EncodingType = string(common.EncodingTypeThriftRW) - } - if len(store.SQL.DecodingTypes) == 0 { - store.SQL.DecodingTypes = []string{ - string(common.EncodingTypeThriftRW), - } - } - c.Persistence.DataStores[k] = store - } - } + c.Persistence.FillDefaults() // TODO: remove this after 0.23 and mention a breaking change in config. if c.ClusterGroupMetadata == nil && c.ClusterMetadata != nil { @@ -477,7 +463,7 @@ func (c *Config) fillDefaults() { log.Println("[WARN] clusterMetadata config is deprecated. Please replace it with clusterGroupMetadata.") } - c.ClusterGroupMetadata.fillDefaults() + c.ClusterGroupMetadata.FillDefaults() // filling publicClient with current cluster's RPC address if empty if c.PublicClient.HostPort == "" && c.ClusterGroupMetadata != nil { diff --git a/common/config/persistence.go b/common/config/persistence.go index d9896e7e1bd..24d720c41a9 100644 --- a/common/config/persistence.go +++ b/common/config/persistence.go @@ -20,7 +20,11 @@ package config -import "fmt" +import ( + "fmt" + + "github.com/uber/cadence/common" +) const ( // StoreTypeSQL refers to sql based storage as persistence store @@ -37,6 +41,36 @@ func (c *Persistence) DefaultStoreType() string { return StoreTypeCassandra } +// FillDefaults populates default values for unspecified fields in persistence config +func (c *Persistence) FillDefaults() { + for k, store := range c.DataStores { + if store.Cassandra != nil && store.NoSQL == nil { + // for backward-compatibility + store.NoSQL = store.Cassandra + store.NoSQL.PluginName = "cassandra" + } + + if store.SQL != nil { + // filling default encodingType/decodingTypes for SQL persistence + if store.SQL.EncodingType == "" { + store.SQL.EncodingType = string(common.EncodingTypeThriftRW) + } + if len(store.SQL.DecodingTypes) == 0 { + store.SQL.DecodingTypes = []string{ + string(common.EncodingTypeThriftRW), + } + } + + if store.SQL.NumShards == 0 { + store.SQL.NumShards = 1 + } + } + + // write changes back to DataStores, as ds is a value object + c.DataStores[k] = store + } +} + // Validate validates the persistence config func (c *Persistence) Validate() error { dbStoreKeys := []string{c.DefaultStore} @@ -45,7 +79,7 @@ func (c *Persistence) Validate() error { dbStoreKeys = append(dbStoreKeys, c.VisibilityStore) } else { if _, ok := c.DataStores[c.AdvancedVisibilityStore]; !ok { - return fmt.Errorf(" Must provide one of VisibilityStore and AdvancedVisibilityStore") + return fmt.Errorf("must provide one of VisibilityStore and AdvancedVisibilityStore") } } @@ -54,13 +88,8 @@ func (c *Persistence) Validate() error { if !ok { return fmt.Errorf("persistence config: missing config for datastore %v", st) } - if ds.Cassandra != nil { - if ds.NoSQL != nil { - return fmt.Errorf("persistence config: datastore %v: only one of Cassandra or NoSQL can be specified", st) - } - // for backward-compatibility - ds.NoSQL = ds.Cassandra - ds.NoSQL.PluginName = "cassandra" + if ds.Cassandra != nil && ds.NoSQL != nil && ds.Cassandra != ds.NoSQL { + return fmt.Errorf("persistence config: datastore %v: only one of Cassandra or NoSQL can be specified", st) } if ds.SQL == nil && ds.NoSQL == nil { return fmt.Errorf("persistence config: datastore %v: must provide config for one of SQL or NoSQL stores", st) @@ -68,12 +97,8 @@ func (c *Persistence) Validate() error { if ds.SQL != nil && ds.NoSQL != nil { return fmt.Errorf("persistence config: datastore %v: only one of SQL or NoSQL can be specified", st) } - if ds.SQL != nil && ds.SQL.NumShards == 0 { - ds.SQL.NumShards = 1 - } - // write changes back to DataStores, as ds is a value object - c.DataStores[st] = ds } + return nil }