From cb5a230d82368a5d8074fd207f1fe115882cc669 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 25 Sep 2025 14:46:39 -0700 Subject: [PATCH 1/4] Hide retry config from public API and change default retry count to 1 - Remove RetryConfig field from public cmab.Config struct - Change DefaultMaxRetries from 3 to 1 for conservative first release - Use hardcoded internal retry config in ExperimentCmabService - Create client-level CmabConfig type for stable public API - Update all tests to use new configuration approach This keeps retry functionality internal while preventing API exposure that might need to change in future releases. --- pkg/client/factory.go | 29 ++++++++++++-- pkg/client/factory_test.go | 27 ++++--------- pkg/cmab/client.go | 2 +- pkg/cmab/config.go | 4 -- pkg/cmab/config_test.go | 2 - .../composite_experiment_service_test.go | 5 --- pkg/decision/experiment_cmab_service.go | 38 ++++--------------- 7 files changed, 41 insertions(+), 66 deletions(-) diff --git a/pkg/client/factory.go b/pkg/client/factory.go index c05f70a2..050b0904 100644 --- a/pkg/client/factory.go +++ b/pkg/client/factory.go @@ -22,6 +22,7 @@ import ( "errors" "time" + "github.com/optimizely/go-sdk/v2/pkg/cache" "github.com/optimizely/go-sdk/v2/pkg/cmab" "github.com/optimizely/go-sdk/v2/pkg/config" "github.com/optimizely/go-sdk/v2/pkg/decide" @@ -37,6 +38,28 @@ import ( "github.com/optimizely/go-sdk/v2/pkg/utils" ) +// CmabConfig holds CMAB configuration options exposed at the client level. +// This provides a stable public API while allowing internal cmab.Config to change. +type CmabConfig struct { + CacheSize int + CacheTTL time.Duration + HTTPTimeout time.Duration + Cache cache.CacheWithRemove // Custom cache implementation (Redis, etc.) +} + +// toCmabConfig converts client-level CmabConfig to internal cmab.Config +func (c *CmabConfig) toCmabConfig() *cmab.Config { + if c == nil { + return nil + } + return &cmab.Config{ + CacheSize: c.CacheSize, + CacheTTL: c.CacheTTL, + HTTPTimeout: c.HTTPTimeout, + Cache: c.Cache, + } +} + // OptimizelyFactory is used to customize and construct an instance of the OptimizelyClient. type OptimizelyFactory struct { SDKKey string @@ -54,7 +77,7 @@ type OptimizelyFactory struct { overrideStore decision.ExperimentOverrideStore userProfileService decision.UserProfileService notificationCenter notification.Center - cmabConfig *cmab.Config + cmabConfig *CmabConfig // ODP segmentsCacheSize int @@ -163,7 +186,7 @@ func (f *OptimizelyFactory) Client(clientOptions ...OptionFunc) (*OptimizelyClie } // Add CMAB config option if provided if f.cmabConfig != nil { - experimentServiceOptions = append(experimentServiceOptions, decision.WithCmabConfig(f.cmabConfig)) + experimentServiceOptions = append(experimentServiceOptions, decision.WithCmabConfig(f.cmabConfig.toCmabConfig())) } compositeExperimentService := decision.NewCompositeExperimentService(f.SDKKey, experimentServiceOptions...) compositeService := decision.NewCompositeService(f.SDKKey, decision.WithCompositeExperimentService(compositeExperimentService)) @@ -327,7 +350,7 @@ func WithTracer(tracer tracing.Tracer) OptionFunc { } // WithCmabConfig sets the CMAB configuration options -func WithCmabConfig(cmabConfig *cmab.Config) OptionFunc { +func WithCmabConfig(cmabConfig *CmabConfig) OptionFunc { return func(f *OptimizelyFactory) { f.cmabConfig = cmabConfig } diff --git a/pkg/client/factory_test.go b/pkg/client/factory_test.go index fe878ec4..d6e35860 100644 --- a/pkg/client/factory_test.go +++ b/pkg/client/factory_test.go @@ -28,7 +28,6 @@ import ( "github.com/stretchr/testify/mock" "github.com/optimizely/go-sdk/v2/pkg/cache" - "github.com/optimizely/go-sdk/v2/pkg/cmab" "github.com/optimizely/go-sdk/v2/pkg/config" "github.com/optimizely/go-sdk/v2/pkg/decide" "github.com/optimizely/go-sdk/v2/pkg/decision" @@ -461,13 +460,10 @@ func TestStaticClientError(t *testing.T) { func TestFactoryWithCmabConfig(t *testing.T) { factory := OptimizelyFactory{} - cmabConfig := cmab.Config{ + cmabConfig := CmabConfig{ CacheSize: 100, CacheTTL: time.Minute, HTTPTimeout: 30 * time.Second, - RetryConfig: &cmab.RetryConfig{ - MaxRetries: 5, - }, } // Test the option function @@ -477,19 +473,14 @@ func TestFactoryWithCmabConfig(t *testing.T) { assert.Equal(t, 100, factory.cmabConfig.CacheSize) assert.Equal(t, time.Minute, factory.cmabConfig.CacheTTL) assert.Equal(t, 30*time.Second, factory.cmabConfig.HTTPTimeout) - assert.NotNil(t, factory.cmabConfig.RetryConfig) - assert.Equal(t, 5, factory.cmabConfig.RetryConfig.MaxRetries) } func TestFactoryCmabConfigPassedToDecisionService(t *testing.T) { // Test that CMAB config is correctly passed to decision service when creating client - cmabConfig := cmab.Config{ + cmabConfig := CmabConfig{ CacheSize: 200, CacheTTL: 2 * time.Minute, HTTPTimeout: 20 * time.Second, - RetryConfig: &cmab.RetryConfig{ - MaxRetries: 3, - }, } factory := OptimizelyFactory{ @@ -501,7 +492,6 @@ func TestFactoryCmabConfigPassedToDecisionService(t *testing.T) { assert.Equal(t, &cmabConfig, factory.cmabConfig) assert.Equal(t, 200, factory.cmabConfig.CacheSize) assert.Equal(t, 2*time.Minute, factory.cmabConfig.CacheTTL) - assert.NotNil(t, factory.cmabConfig.RetryConfig) } func TestFactoryOptionFunctions(t *testing.T) { @@ -512,19 +502,19 @@ func TestFactoryOptionFunctions(t *testing.T) { WithSegmentsCacheSize(100)(factory) WithSegmentsCacheTimeout(5 * time.Second)(factory) WithOdpDisabled(true)(factory) - WithCmabConfig(&cmab.Config{CacheSize: 50})(factory) + WithCmabConfig(&CmabConfig{CacheSize: 50})(factory) // Verify options were set assert.Equal(t, "test_token", factory.DatafileAccessToken) assert.Equal(t, 100, factory.segmentsCacheSize) assert.Equal(t, 5*time.Second, factory.segmentsCacheTimeout) assert.True(t, factory.odpDisabled) - assert.Equal(t, &cmab.Config{CacheSize: 50}, factory.cmabConfig) + assert.Equal(t, &CmabConfig{CacheSize: 50}, factory.cmabConfig) } func TestWithCmabConfigOption(t *testing.T) { factory := &OptimizelyFactory{} - testConfig := cmab.Config{ + testConfig := CmabConfig{ CacheSize: 200, CacheTTL: 2 * time.Minute, } @@ -534,13 +524,10 @@ func TestWithCmabConfigOption(t *testing.T) { func TestClientWithCmabConfig(t *testing.T) { // Test client creation with non-empty CMAB config (tests reflect.DeepEqual path) - cmabConfig := cmab.Config{ + cmabConfig := CmabConfig{ CacheSize: 200, CacheTTL: 5 * time.Minute, HTTPTimeout: 30 * time.Second, - RetryConfig: &cmab.RetryConfig{ - MaxRetries: 5, - }, } factory := OptimizelyFactory{ @@ -559,7 +546,7 @@ func TestClientWithCmabConfig(t *testing.T) { func TestClientWithEmptyCmabConfig(t *testing.T) { // Test client creation with empty CMAB config (tests reflect.DeepEqual returns true) - emptyCmabConfig := cmab.Config{} + emptyCmabConfig := CmabConfig{} factory := OptimizelyFactory{ SDKKey: "test_sdk_key", diff --git a/pkg/cmab/client.go b/pkg/cmab/client.go index bfb7c9fa..588e518e 100644 --- a/pkg/cmab/client.go +++ b/pkg/cmab/client.go @@ -35,7 +35,7 @@ var CMABPredictionEndpoint = "https://prediction.cmab.optimizely.com/predict/%s" const ( // DefaultMaxRetries is the default number of retries for CMAB requests - DefaultMaxRetries = 3 + DefaultMaxRetries = 1 // DefaultInitialBackoff is the default initial backoff duration DefaultInitialBackoff = 100 * time.Millisecond // DefaultMaxBackoff is the default maximum backoff duration diff --git a/pkg/cmab/config.go b/pkg/cmab/config.go index 746b3f4f..e81d0c28 100644 --- a/pkg/cmab/config.go +++ b/pkg/cmab/config.go @@ -38,7 +38,6 @@ type Config struct { CacheSize int CacheTTL time.Duration HTTPTimeout time.Duration - RetryConfig *RetryConfig Cache cache.CacheWithRemove // Custom cache implementation (Redis, etc.) } @@ -48,8 +47,5 @@ func NewDefaultConfig() Config { CacheSize: DefaultCacheSize, CacheTTL: DefaultCacheTTL, HTTPTimeout: DefaultHTTPTimeout, - RetryConfig: &RetryConfig{ - MaxRetries: DefaultMaxRetries, - }, } } diff --git a/pkg/cmab/config_test.go b/pkg/cmab/config_test.go index bbc643d4..8311f40b 100644 --- a/pkg/cmab/config_test.go +++ b/pkg/cmab/config_test.go @@ -29,8 +29,6 @@ func TestNewDefaultConfig(t *testing.T) { assert.Equal(t, DefaultCacheSize, config.CacheSize) assert.Equal(t, DefaultCacheTTL, config.CacheTTL) assert.Equal(t, DefaultHTTPTimeout, config.HTTPTimeout) - assert.NotNil(t, config.RetryConfig) - assert.Equal(t, DefaultMaxRetries, config.RetryConfig.MaxRetries) assert.Nil(t, config.Cache) // Should be nil by default } diff --git a/pkg/decision/composite_experiment_service_test.go b/pkg/decision/composite_experiment_service_test.go index fa65d3a1..177b216f 100644 --- a/pkg/decision/composite_experiment_service_test.go +++ b/pkg/decision/composite_experiment_service_test.go @@ -252,9 +252,6 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCmab CacheSize: 200, CacheTTL: 5 * time.Minute, HTTPTimeout: 30 * time.Second, - RetryConfig: &cmab.RetryConfig{ - MaxRetries: 5, - }, } compositeExperimentService := NewCompositeExperimentService("test-sdk-key", @@ -266,8 +263,6 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCmab s.Equal(200, compositeExperimentService.cmabConfig.CacheSize) // From config s.Equal(5*time.Minute, compositeExperimentService.cmabConfig.CacheTTL) // From config s.Equal(30*time.Second, compositeExperimentService.cmabConfig.HTTPTimeout) // From config - s.NotNil(compositeExperimentService.cmabConfig.RetryConfig) - s.Equal(5, compositeExperimentService.cmabConfig.RetryConfig.MaxRetries) // From config // Verify service order s.Equal(3, len(compositeExperimentService.experimentServices)) diff --git a/pkg/decision/experiment_cmab_service.go b/pkg/decision/experiment_cmab_service.go index 53dd9c23..0691f996 100644 --- a/pkg/decision/experiment_cmab_service.go +++ b/pkg/decision/experiment_cmab_service.go @@ -50,7 +50,6 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma var cacheSize int var cacheTTL time.Duration var httpTimeout time.Duration - var retryConfig *cmab.RetryConfig var customCache cache.CacheWithRemove if config == nil { @@ -58,12 +57,6 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma cacheSize = cmab.DefaultCacheSize cacheTTL = cmab.DefaultCacheTTL httpTimeout = cmab.DefaultHTTPTimeout - retryConfig = &cmab.RetryConfig{ - MaxRetries: cmab.DefaultMaxRetries, - InitialBackoff: cmab.DefaultInitialBackoff, - MaxBackoff: cmab.DefaultMaxBackoff, - BackoffMultiplier: cmab.DefaultBackoffMultiplier, - } } else { // Config is not nil, use defaults for zero values customCache = config.Cache @@ -82,31 +75,14 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma if httpTimeout == 0 { httpTimeout = cmab.DefaultHTTPTimeout } + } - // Handle retry config - if config.RetryConfig == nil { - retryConfig = &cmab.RetryConfig{ - MaxRetries: cmab.DefaultMaxRetries, - InitialBackoff: cmab.DefaultInitialBackoff, - MaxBackoff: cmab.DefaultMaxBackoff, - BackoffMultiplier: cmab.DefaultBackoffMultiplier, - } - } else { - retryConfig = config.RetryConfig - // Apply defaults for zero values in provided RetryConfig - if retryConfig.MaxRetries == 0 { - retryConfig.MaxRetries = cmab.DefaultMaxRetries - } - if retryConfig.InitialBackoff == 0 { - retryConfig.InitialBackoff = cmab.DefaultInitialBackoff - } - if retryConfig.MaxBackoff == 0 { - retryConfig.MaxBackoff = cmab.DefaultMaxBackoff - } - if retryConfig.BackoffMultiplier == 0 { - retryConfig.BackoffMultiplier = cmab.DefaultBackoffMultiplier - } - } + // Always use hardcoded retry config (not exposed in public API) + retryConfig := &cmab.RetryConfig{ + MaxRetries: cmab.DefaultMaxRetries, + InitialBackoff: cmab.DefaultInitialBackoff, + MaxBackoff: cmab.DefaultMaxBackoff, + BackoffMultiplier: cmab.DefaultBackoffMultiplier, } // Use custom cache if provided, otherwise create default LRU cache From bb1804c4f4cbab82e1d48543d430dab3ca217508 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 25 Sep 2025 15:01:43 -0700 Subject: [PATCH 2/4] format --- pkg/cmab/service_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index 141051ef..7c13edf8 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -845,7 +845,6 @@ func (s *CmabServiceTestSuite) TestGetDecisionApiError() { s.mockClient.AssertExpectations(s.T()) } - // TestLockStripingDistribution verifies that different user/rule combinations // use different locks to allow for better concurrency func TestLockStripingDistribution(t *testing.T) { From 602a4cb4c9741327523e40a9b80f991abcc23f16 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 26 Sep 2025 10:34:42 -0700 Subject: [PATCH 3/4] Fix retry config handling per reviewer feedback Keep existing code structure instead of hardcoding retry config. Since RetryConfig is no longer in public API, always use defaults but maintain the same if/else structure for consistency. --- pkg/decision/experiment_cmab_service.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/pkg/decision/experiment_cmab_service.go b/pkg/decision/experiment_cmab_service.go index 0691f996..1667d51f 100644 --- a/pkg/decision/experiment_cmab_service.go +++ b/pkg/decision/experiment_cmab_service.go @@ -50,6 +50,7 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma var cacheSize int var cacheTTL time.Duration var httpTimeout time.Duration + var retryConfig *cmab.RetryConfig var customCache cache.CacheWithRemove if config == nil { @@ -57,6 +58,12 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma cacheSize = cmab.DefaultCacheSize cacheTTL = cmab.DefaultCacheTTL httpTimeout = cmab.DefaultHTTPTimeout + retryConfig = &cmab.RetryConfig{ + MaxRetries: cmab.DefaultMaxRetries, + InitialBackoff: cmab.DefaultInitialBackoff, + MaxBackoff: cmab.DefaultMaxBackoff, + BackoffMultiplier: cmab.DefaultBackoffMultiplier, + } } else { // Config is not nil, use defaults for zero values customCache = config.Cache @@ -75,14 +82,14 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma if httpTimeout == 0 { httpTimeout = cmab.DefaultHTTPTimeout } - } - // Always use hardcoded retry config (not exposed in public API) - retryConfig := &cmab.RetryConfig{ - MaxRetries: cmab.DefaultMaxRetries, - InitialBackoff: cmab.DefaultInitialBackoff, - MaxBackoff: cmab.DefaultMaxBackoff, - BackoffMultiplier: cmab.DefaultBackoffMultiplier, + // Handle retry config - since RetryConfig is no longer in public API, always use defaults + retryConfig = &cmab.RetryConfig{ + MaxRetries: cmab.DefaultMaxRetries, + InitialBackoff: cmab.DefaultInitialBackoff, + MaxBackoff: cmab.DefaultMaxBackoff, + BackoffMultiplier: cmab.DefaultBackoffMultiplier, + } } // Use custom cache if provided, otherwise create default LRU cache From 93e9761670b3aa3686118a1d8bb0a68dc7e254ad Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 26 Sep 2025 12:59:49 -0700 Subject: [PATCH 4/4] Revert internal changes per reviewer feedback - keep only factory changes --- pkg/cmab/config.go | 4 +++ pkg/cmab/config_test.go | 2 ++ .../composite_experiment_service_test.go | 5 ++++ pkg/decision/experiment_cmab_service.go | 29 +++++++++++++++---- 4 files changed, 34 insertions(+), 6 deletions(-) diff --git a/pkg/cmab/config.go b/pkg/cmab/config.go index e81d0c28..746b3f4f 100644 --- a/pkg/cmab/config.go +++ b/pkg/cmab/config.go @@ -38,6 +38,7 @@ type Config struct { CacheSize int CacheTTL time.Duration HTTPTimeout time.Duration + RetryConfig *RetryConfig Cache cache.CacheWithRemove // Custom cache implementation (Redis, etc.) } @@ -47,5 +48,8 @@ func NewDefaultConfig() Config { CacheSize: DefaultCacheSize, CacheTTL: DefaultCacheTTL, HTTPTimeout: DefaultHTTPTimeout, + RetryConfig: &RetryConfig{ + MaxRetries: DefaultMaxRetries, + }, } } diff --git a/pkg/cmab/config_test.go b/pkg/cmab/config_test.go index 8311f40b..bbc643d4 100644 --- a/pkg/cmab/config_test.go +++ b/pkg/cmab/config_test.go @@ -29,6 +29,8 @@ func TestNewDefaultConfig(t *testing.T) { assert.Equal(t, DefaultCacheSize, config.CacheSize) assert.Equal(t, DefaultCacheTTL, config.CacheTTL) assert.Equal(t, DefaultHTTPTimeout, config.HTTPTimeout) + assert.NotNil(t, config.RetryConfig) + assert.Equal(t, DefaultMaxRetries, config.RetryConfig.MaxRetries) assert.Nil(t, config.Cache) // Should be nil by default } diff --git a/pkg/decision/composite_experiment_service_test.go b/pkg/decision/composite_experiment_service_test.go index 177b216f..fa65d3a1 100644 --- a/pkg/decision/composite_experiment_service_test.go +++ b/pkg/decision/composite_experiment_service_test.go @@ -252,6 +252,9 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCmab CacheSize: 200, CacheTTL: 5 * time.Minute, HTTPTimeout: 30 * time.Second, + RetryConfig: &cmab.RetryConfig{ + MaxRetries: 5, + }, } compositeExperimentService := NewCompositeExperimentService("test-sdk-key", @@ -263,6 +266,8 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCmab s.Equal(200, compositeExperimentService.cmabConfig.CacheSize) // From config s.Equal(5*time.Minute, compositeExperimentService.cmabConfig.CacheTTL) // From config s.Equal(30*time.Second, compositeExperimentService.cmabConfig.HTTPTimeout) // From config + s.NotNil(compositeExperimentService.cmabConfig.RetryConfig) + s.Equal(5, compositeExperimentService.cmabConfig.RetryConfig.MaxRetries) // From config // Verify service order s.Equal(3, len(compositeExperimentService.experimentServices)) diff --git a/pkg/decision/experiment_cmab_service.go b/pkg/decision/experiment_cmab_service.go index 1667d51f..53dd9c23 100644 --- a/pkg/decision/experiment_cmab_service.go +++ b/pkg/decision/experiment_cmab_service.go @@ -83,12 +83,29 @@ func NewExperimentCmabService(sdkKey string, config *cmab.Config) *ExperimentCma httpTimeout = cmab.DefaultHTTPTimeout } - // Handle retry config - since RetryConfig is no longer in public API, always use defaults - retryConfig = &cmab.RetryConfig{ - MaxRetries: cmab.DefaultMaxRetries, - InitialBackoff: cmab.DefaultInitialBackoff, - MaxBackoff: cmab.DefaultMaxBackoff, - BackoffMultiplier: cmab.DefaultBackoffMultiplier, + // Handle retry config + if config.RetryConfig == nil { + retryConfig = &cmab.RetryConfig{ + MaxRetries: cmab.DefaultMaxRetries, + InitialBackoff: cmab.DefaultInitialBackoff, + MaxBackoff: cmab.DefaultMaxBackoff, + BackoffMultiplier: cmab.DefaultBackoffMultiplier, + } + } else { + retryConfig = config.RetryConfig + // Apply defaults for zero values in provided RetryConfig + if retryConfig.MaxRetries == 0 { + retryConfig.MaxRetries = cmab.DefaultMaxRetries + } + if retryConfig.InitialBackoff == 0 { + retryConfig.InitialBackoff = cmab.DefaultInitialBackoff + } + if retryConfig.MaxBackoff == 0 { + retryConfig.MaxBackoff = cmab.DefaultMaxBackoff + } + if retryConfig.BackoffMultiplier == 0 { + retryConfig.BackoffMultiplier = cmab.DefaultBackoffMultiplier + } } }