Sets default activity maximum retry interval to be 100x minimum retry interval (#630)

Fixes situation where user sets an initial retry interval that is greater than 100 seconds, but doesn't explicitly set the maximum retry interval, which will lead to a validation failure when they try to start an activity.

Author: mastermanu

common/defaultActivityRetrySettings.go

@@ -0,0 +1,10 @@
+package common
+
+// DefaultActivityRetrySettings indicates what the "default" activity retry settings
+// are of it is not specified on an Activity
+type DefaultActivityRetrySettings struct {
+	InitialIntervalInSeconds   int32
+	MaximumIntervalCoefficient float64
+	BackoffCoefficient         float64
+	MaximumAttempts            int32
+}

common/util.go

@@ -368,32 +368,32 @@ func SortInt64Slice(slice []int64) {
 }
 
 // EnsureRetryPolicyDefaults ensures the policy subfields, if not explicitly set, are set to the specified defaults
-func EnsureRetryPolicyDefaults(originalPolicy *commonpb.RetryPolicy, defaultPolicy *commonpb.RetryPolicy) *commonpb.RetryPolicy {
-	if originalPolicy == nil {
-		return defaultPolicy
-	}
-
-	merged := &commonpb.RetryPolicy{
-		BackoffCoefficient:       originalPolicy.GetBackoffCoefficient(),
-		InitialIntervalInSeconds: originalPolicy.GetInitialIntervalInSeconds(),
-		MaximumIntervalInSeconds: originalPolicy.GetMaximumIntervalInSeconds(),
-		MaximumAttempts:          originalPolicy.GetMaximumAttempts(),
+func EnsureRetryPolicyDefaults(originalPolicy *commonpb.RetryPolicy, defaultSettings DefaultActivityRetrySettings) *commonpb.RetryPolicy {
+	var merged *commonpb.RetryPolicy = &commonpb.RetryPolicy{}
+
+	if originalPolicy != nil {
+		merged = &commonpb.RetryPolicy{
+			BackoffCoefficient:       originalPolicy.GetBackoffCoefficient(),
+			InitialIntervalInSeconds: originalPolicy.GetInitialIntervalInSeconds(),
+			MaximumIntervalInSeconds: originalPolicy.GetMaximumIntervalInSeconds(),
+			MaximumAttempts:          originalPolicy.GetMaximumAttempts(),
+		}
 	}
 
 	if merged.GetMaximumAttempts() == 0 {
-		merged.MaximumAttempts = defaultPolicy.GetMaximumAttempts()
+		merged.MaximumAttempts = int32(defaultSettings.MaximumAttempts)
 	}
 
 	if merged.GetInitialIntervalInSeconds() == 0 {
-		merged.InitialIntervalInSeconds = defaultPolicy.GetInitialIntervalInSeconds()
+		merged.InitialIntervalInSeconds = int32(defaultSettings.InitialIntervalInSeconds)
 	}
 
 	if merged.GetMaximumIntervalInSeconds() == 0 {
-		merged.MaximumIntervalInSeconds = defaultPolicy.GetMaximumIntervalInSeconds()
+		merged.MaximumIntervalInSeconds = int32(defaultSettings.MaximumIntervalCoefficient * float64(merged.GetInitialIntervalInSeconds()))
 	}
 
 	if merged.GetBackoffCoefficient() == 0 {
-		merged.BackoffCoefficient = defaultPolicy.GetBackoffCoefficient()
+		merged.BackoffCoefficient = defaultSettings.BackoffCoefficient
 	}
 
 	return merged

common/util_test.go

@@ -89,10 +89,17 @@ func TestValidateRetryPolicy(t *testing.T) {
 }
 
 func TestEnsureRetryPolicyDefaults(t *testing.T) {
+	defaultActivityRetrySettings := DefaultActivityRetrySettings{
+		InitialIntervalInSeconds:   1,
+		MaximumIntervalCoefficient: 100,
+		BackoffCoefficient:         2.0,
+		MaximumAttempts:            120,
+	}
+
 	defaultRetryPolicy := &commonpb.RetryPolicy{
 		InitialIntervalInSeconds: 1,
 		MaximumIntervalInSeconds: 100,
-		BackoffCoefficient:       2,
+		BackoffCoefficient:       2.0,
 		MaximumAttempts:          120,
 	}
 
@@ -118,7 +125,7 @@ func TestEnsureRetryPolicyDefaults(t *testing.T) {
 			},
 			want: &commonpb.RetryPolicy{
 				InitialIntervalInSeconds: 2,
-				MaximumIntervalInSeconds: 100,
+				MaximumIntervalInSeconds: 200,
 				BackoffCoefficient:       2,
 				MaximumAttempts:          120,
 			},
@@ -163,7 +170,7 @@ func TestEnsureRetryPolicyDefaults(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			got := EnsureRetryPolicyDefaults(tt.input, defaultRetryPolicy)
+			got := EnsureRetryPolicyDefaults(tt.input, defaultActivityRetrySettings)
 			assert.Equal(t, tt.want, got)
 		})
 	}

config/dynamicconfig/development.yaml

@@ -24,7 +24,7 @@ system.advancedVisibilityWritingMode:
   constraints: {}
 history.defaultActivityRetryPolicy:
 - value:
-    InitialRetryIntervalInSeconds: 1
-    MaximumRetryIntervalInSeconds: 120
-    ExponentialBackoffCoefficient: 2.0
+    InitialIntervalInSeconds: 1
+    MaximumIntervalCoefficient: 100.0
+    BackoffCoefficient: 2.0
     MaximumAttempts: 0

service/history/commandChecker.go

@@ -51,11 +51,11 @@ import (
 
 type (
 	commandAttrValidator struct {
-		namespaceCache             cache.NamespaceCache
-		config                     *Config
-		maxIDLengthLimit           int
-		searchAttributesValidator  *validator.SearchAttributesValidator
-		defaultActivityRetryPolicy *commonpb.RetryPolicy
+		namespaceCache               cache.NamespaceCache
+		config                       *Config
+		maxIDLengthLimit             int
+		searchAttributesValidator    *validator.SearchAttributesValidator
+		defaultActivityRetrySettings common.DefaultActivityRetrySettings
 	}
 
 	workflowSizeChecker struct {
@@ -96,7 +96,7 @@ func newCommandAttrValidator(
 			config.SearchAttributesSizeOfValueLimit,
 			config.SearchAttributesTotalSizeLimit,
 		),
-		defaultActivityRetryPolicy: fromConfigToActivityRetryPolicy(config.DefaultActivityRetryPolicy()),
+		defaultActivityRetrySettings: fromConfigToDefaultActivityRetrySettings(config.DefaultActivityRetryPolicy()),
 	}
 }
 
@@ -652,12 +652,7 @@ func (v *commandAttrValidator) validateTaskQueue(
 }
 
 func (v *commandAttrValidator) validateActivityRetryPolicy(attributes *commandpb.ScheduleActivityTaskCommandAttributes) error {
-	if attributes.RetryPolicy == nil {
-		attributes.RetryPolicy = v.defaultActivityRetryPolicy
-		return nil
-	}
-
-	attributes.RetryPolicy = common.EnsureRetryPolicyDefaults(attributes.RetryPolicy, v.defaultActivityRetryPolicy)
+	attributes.RetryPolicy = common.EnsureRetryPolicyDefaults(attributes.RetryPolicy, v.defaultActivityRetrySettings)
 	return common.ValidateRetryPolicy(attributes.RetryPolicy)
 }
 

service/history/commandChecker_test.go

@@ -587,7 +587,7 @@ func (s *commandAttrValidatorSuite) TestValidateActivityRetryPolicy() {
 			},
 		},
 		{
-			name: "partial override set policy",
+			name: "partial override of fields",
 			input: &commonpb.RetryPolicy{
 				InitialIntervalInSeconds: 0,
 				BackoffCoefficient:       1.2,
@@ -601,6 +601,19 @@ func (s *commandAttrValidatorSuite) TestValidateActivityRetryPolicy() {
 				MaximumAttempts:          7,
 			},
 		},
+		{
+			name: "set expected max interval if only init interval set",
+			input: &commonpb.RetryPolicy{
+				InitialIntervalInSeconds: 3,
+				MaximumIntervalInSeconds: 0,
+			},
+			want: &commonpb.RetryPolicy{
+				InitialIntervalInSeconds: 3,
+				BackoffCoefficient:       2,
+				MaximumIntervalInSeconds: 300,
+				MaximumAttempts:          0,
+			},
+		},
 		{
 			name: "override all defaults",
 			input: &commonpb.RetryPolicy{

service/history/retry.go

@@ -29,14 +29,18 @@ import (
 	"math"
 	"time"
 
-	commonpb "go.temporal.io/api/common/v1"
 	enumspb "go.temporal.io/api/enums/v1"
 	failurepb "go.temporal.io/api/failure/v1"
 
 	"go.temporal.io/server/common"
 	"go.temporal.io/server/common/backoff"
 )
 
+const defaultInitialIntervalInSeconds = 1
+const defaultMaximumIntervalCoefficient = 100.0
+const defaultBackoffCoefficient = 2.0
+const defaultMaximumAttempts = 0
+
 func getBackoffInterval(
 	now time.Time,
 	expirationTime time.Time,
@@ -133,43 +137,49 @@ func isRetryable(failure *failurepb.Failure, nonRetryableTypes []string) bool {
 
 func getDefaultActivityRetryPolicyConfigOptions() map[string]interface{} {
 	return map[string]interface{}{
-		"InitialRetryIntervalInSeconds": 1,
-		"MaximumRetryIntervalInSeconds": 100,
-		"ExponentialBackoffCoefficient": 2.0,
-		"MaximumAttempts":               0,
+		"InitialIntervalInSeconds": 1,
+		"MaximumIntervalInSeconds": 100,
+		"BackoffCoefficient":       2.0,
+		"MaximumAttempts":          0,
 	}
 }
 
-func fromConfigToActivityRetryPolicy(options map[string]interface{}) *commonpb.RetryPolicy {
-	retryPolicy := &commonpb.RetryPolicy{}
-	initialRetryInterval, ok := options["InitialRetryIntervalInSeconds"]
+func fromConfigToDefaultActivityRetrySettings(options map[string]interface{}) common.DefaultActivityRetrySettings {
+	defaultSettings := common.DefaultActivityRetrySettings{
+		InitialIntervalInSeconds:   defaultInitialIntervalInSeconds,
+		MaximumIntervalCoefficient: defaultMaximumIntervalCoefficient,
+		BackoffCoefficient:         defaultBackoffCoefficient,
+		MaximumAttempts:            defaultMaximumAttempts,
+	}
+
+	initialIntervalInSeconds, ok := options["InitialIntervalInSeconds"]
 	if ok {
-		retryPolicy.InitialIntervalInSeconds = int32(initialRetryInterval.(int))
+		defaultSettings.InitialIntervalInSeconds = int32(initialIntervalInSeconds.(int))
 	}
 
-	maxRetryInterval, ok := options["MaximumRetryIntervalInSeconds"]
+	maximumIntervalCoefficient, ok := options["MaximumIntervalCoefficient"]
 	if ok {
-		retryPolicy.MaximumIntervalInSeconds = int32(maxRetryInterval.(int))
+		defaultSettings.MaximumIntervalCoefficient = maximumIntervalCoefficient.(float64)
 	}
 
-	exponentialBackoffCoefficient, ok := options["ExponentialBackoffCoefficient"]
+	backoffCoefficient, ok := options["BackoffCoefficient"]
 	if ok {
-		retryPolicy.BackoffCoefficient = exponentialBackoffCoefficient.(float64)
+		defaultSettings.BackoffCoefficient = backoffCoefficient.(float64)
 	}
 
 	maximumAttempts, ok := options["MaximumAttempts"]
 	if ok {
-		retryPolicy.MaximumAttempts = int32(maximumAttempts.(int))
+		defaultSettings.MaximumAttempts = int32(maximumAttempts.(int))
 	}
 
-	err := common.ValidateRetryPolicy(retryPolicy)
+	err := common.ValidateRetryPolicy(common.EnsureRetryPolicyDefaults(nil, defaultSettings))
 	if err != nil {
 		panic(
 			fmt.Sprintf(
-				"Bad Default Activity Retry Policy defined: %+v failed validation %v",
-				retryPolicy,
+				"Bad Default Activity Retry Settings defined: %+v failed validation %v",
+				defaultSettings,
 				err))
 	}
 
-	return retryPolicy
+	return defaultSettings
 }

service/history/retry_test.go

@@ -398,15 +398,15 @@ func Test_NextRetry(t *testing.T) {
 
 func Test_FromConfigToActivityRetryPolicy(t *testing.T) {
 	options := map[string]interface{}{
-		"InitialRetryIntervalInSeconds": 2,
-		"MaximumRetryIntervalInSeconds": 200,
-		"ExponentialBackoffCoefficient": 4.0,
-		"MaximumAttempts":               5,
+		"InitialIntervalInSeconds":   2,
+		"MaximumIntervalCoefficient": 100.0,
+		"BackoffCoefficient":         4.0,
+		"MaximumAttempts":            5,
 	}
 
-	policy := fromConfigToActivityRetryPolicy(options)
-	assert.Equal(t, int32(2), policy.GetInitialIntervalInSeconds())
-	assert.Equal(t, int32(200), policy.GetMaximumIntervalInSeconds())
-	assert.Equal(t, 4.0, policy.GetBackoffCoefficient())
-	assert.Equal(t, int32(5), policy.GetMaximumAttempts())
+	defaultSettings := fromConfigToDefaultActivityRetrySettings(options)
+	assert.Equal(t, int32(2), defaultSettings.InitialIntervalInSeconds)
+	assert.Equal(t, 100.0, defaultSettings.MaximumIntervalCoefficient)
+	assert.Equal(t, 4.0, defaultSettings.BackoffCoefficient)
+	assert.Equal(t, int32(5), defaultSettings.MaximumAttempts)
 }