Fixes issue where NonRetryableErrorTypes was getting dropped during a…

…ctivity info validation
temporalio · Jul 30, 2020 · 22ec320 · 22ec320
1 parent 10de202
commit 22ec320
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 32 deletions.
diff --git a/common/defaultActivityRetrySettings.go b/common/defaultActivityRetrySettings.go
@@ -1,7 +1,7 @@
 package common
 
 // DefaultActivityRetrySettings indicates what the "default" activity retry settings
-// are of it is not specified on an Activity
+// are if it is not specified on an Activity
 type DefaultActivityRetrySettings struct {
 	InitialIntervalInSeconds   int32
 	MaximumIntervalCoefficient float64

diff --git a/common/util.go b/common/util.go
@@ -368,35 +368,22 @@ func SortInt64Slice(slice []int64) {
 }
 
 // EnsureRetryPolicyDefaults ensures the policy subfields, if not explicitly set, are set to the specified defaults
-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 = int32(defaultSettings.MaximumAttempts)
+func EnsureRetryPolicyDefaults(originalPolicy *commonpb.RetryPolicy, defaultSettings DefaultActivityRetrySettings) {
+	if originalPolicy.GetMaximumAttempts() == 0 {
+		originalPolicy.MaximumAttempts = int32(defaultSettings.MaximumAttempts)
 	}
 
-	if merged.GetInitialIntervalInSeconds() == 0 {
-		merged.InitialIntervalInSeconds = int32(defaultSettings.InitialIntervalInSeconds)
+	if originalPolicy.GetInitialIntervalInSeconds() == 0 {
+		originalPolicy.InitialIntervalInSeconds = int32(defaultSettings.InitialIntervalInSeconds)
 	}
 
-	if merged.GetMaximumIntervalInSeconds() == 0 {
-		merged.MaximumIntervalInSeconds = int32(defaultSettings.MaximumIntervalCoefficient * float64(merged.GetInitialIntervalInSeconds()))
+	if originalPolicy.GetMaximumIntervalInSeconds() == 0 {
+		originalPolicy.MaximumIntervalInSeconds = int32(defaultSettings.MaximumIntervalCoefficient * float64(originalPolicy.GetInitialIntervalInSeconds()))
 	}
 
-	if merged.GetBackoffCoefficient() == 0 {
-		merged.BackoffCoefficient = defaultSettings.BackoffCoefficient
+	if originalPolicy.GetBackoffCoefficient() == 0 {
+		originalPolicy.BackoffCoefficient = defaultSettings.BackoffCoefficient
 	}
-
-	return merged
 }
 
 // ValidateRetryPolicy validates a retry policy

diff --git a/common/util_test.go b/common/util_test.go
@@ -108,11 +108,6 @@ func TestEnsureRetryPolicyDefaults(t *testing.T) {
 		input *commonpb.RetryPolicy
 		want  *commonpb.RetryPolicy
 	}{
-		{
-			name:  "nil policy is okay",
-			input: nil,
-			want:  defaultRetryPolicy,
-		},
 		{
 			name:  "default fields are set ",
 			input: &commonpb.RetryPolicy{},
@@ -166,12 +161,25 @@ func TestEnsureRetryPolicyDefaults(t *testing.T) {
 				MaximumAttempts:          49,
 			},
 		},
+		{
+			name: "non-retryable errors are set",
+			input: &commonpb.RetryPolicy{
+				NonRetryableErrorTypes: []string{"testFailureType"},
+			},
+			want: &commonpb.RetryPolicy{
+				InitialIntervalInSeconds: 1,
+				MaximumIntervalInSeconds: 100,
+				BackoffCoefficient:       2.0,
+				MaximumAttempts:          120,
+				NonRetryableErrorTypes:   []string{"testFailureType"},
+			},
+		},
 	}
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			got := EnsureRetryPolicyDefaults(tt.input, defaultActivityRetrySettings)
-			assert.Equal(t, tt.want, got)
+			EnsureRetryPolicyDefaults(tt.input, defaultActivityRetrySettings)
+			assert.Equal(t, tt.want, tt.input)
 		})
 	}
 }
diff --git a/service/history/commandChecker.go b/service/history/commandChecker.go
@@ -652,7 +652,11 @@ func (v *commandAttrValidator) validateTaskQueue(
 }
 
 func (v *commandAttrValidator) validateActivityRetryPolicy(attributes *commandpb.ScheduleActivityTaskCommandAttributes) error {
-	attributes.RetryPolicy = common.EnsureRetryPolicyDefaults(attributes.RetryPolicy, v.defaultActivityRetrySettings)
+	if attributes.RetryPolicy == nil {
+		attributes.RetryPolicy = &commonpb.RetryPolicy{}
+	}
+
+	common.EnsureRetryPolicyDefaults(attributes.RetryPolicy, v.defaultActivityRetrySettings)
 	return common.ValidateRetryPolicy(attributes.RetryPolicy)
 }
 

diff --git a/service/history/retry.go b/service/history/retry.go
@@ -29,6 +29,7 @@ import (
 	"math"
 	"time"
 
+	commonpb "go.temporal.io/api/common/v1"
 	enumspb "go.temporal.io/api/enums/v1"
 	failurepb "go.temporal.io/api/failure/v1"
 
@@ -172,7 +173,9 @@ func fromConfigToDefaultActivityRetrySettings(options map[string]interface{}) co
 		defaultSettings.MaximumAttempts = int32(maximumAttempts.(int))
 	}
 
-	err := common.ValidateRetryPolicy(common.EnsureRetryPolicyDefaults(nil, defaultSettings))
+	var empty commonpb.RetryPolicy
+	common.EnsureRetryPolicyDefaults(&empty, defaultSettings)
+	err := common.ValidateRetryPolicy(&empty)
 	if err != nil {
 		panic(
 			fmt.Sprintf(