From ae974aa30e4a0f751a8fb66b8dd19418fa561ed2 Mon Sep 17 00:00:00 2001 From: Carly de Frondeville Date: Wed, 27 Nov 2024 09:08:08 -0800 Subject: [PATCH 1/3] handle error when loading empty build ids keywordlist --- .../history/workflow/mutable_state_impl.go | 22 +++++- .../workflow/mutable_state_impl_test.go | 76 ++++--------------- 2 files changed, 31 insertions(+), 67 deletions(-) diff --git a/service/history/workflow/mutable_state_impl.go b/service/history/workflow/mutable_state_impl.go index a79eebc8a5e..def60cec901 100644 --- a/service/history/workflow/mutable_state_impl.go +++ b/service/history/workflow/mutable_state_impl.go @@ -2407,6 +2407,7 @@ func (ms *MutableStateImpl) AddWorkflowTaskScheduledEvent( if err := ms.checkMutability(opTag); err != nil { return nil, err } + ms.logInfo("AddScheduled") return ms.workflowTaskManager.AddWorkflowTaskScheduledEvent(bypassTaskGeneration, workflowTaskType) } @@ -2453,6 +2454,7 @@ func (ms *MutableStateImpl) AddWorkflowTaskStartedEvent( if err := ms.checkMutability(opTag); err != nil { return nil, nil, err } + ms.logInfo("AddStarted") return ms.workflowTaskManager.AddWorkflowTaskStartedEvent(scheduledEventID, requestID, taskQueue, identity, versioningStamp, redirectInfo, skipVersioningCheck) } @@ -2678,11 +2680,15 @@ func (ms *MutableStateImpl) loadBuildIds() ([]string, error) { if err != nil { return nil, err } - searchAttributeValues, ok := decoded.([]string) - if !ok { - return nil, serviceerror.NewInternal("invalid search attribute value stored for BuildIds") + if decoded != nil { + searchAttributeValues, ok := decoded.([]string) + if !ok { + return nil, serviceerror.NewInternal("invalid search attribute value stored for BuildIds") + } + return searchAttributeValues, nil } - return searchAttributeValues, nil + return make([]string, 0), nil + } // getPinnedDeployment returns nil if the workflow is not pinned. If there is a pinned override, @@ -2706,9 +2712,11 @@ func (ms *MutableStateImpl) addBuildIdToLoadedSearchAttribute( ) []string { var newValues []string effectiveBehavior := ms.GetEffectiveVersioningBehavior() + ms.logInfo(fmt.Sprintf("effectiveBehavior: %s", effectiveBehavior.String())) if !stamp.GetUseVersioning() && effectiveBehavior == enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED { // unversioned workflows may still have non-nil deployment, so we don't check deployment newValues = append(newValues, worker_versioning.UnversionedSearchAttribute) } else if effectiveBehavior == enumspb.VERSIONING_BEHAVIOR_PINNED { + ms.logInfo("appended pinned SA") newValues = append(newValues, worker_versioning.PinnedBuildIdSearchAttribute(ms.getPinnedDeployment())) } else if ms.GetAssignedBuildId() != "" { newValues = append(newValues, worker_versioning.AssignedBuildIdSearchAttribute(ms.GetAssignedBuildId())) @@ -2732,10 +2740,12 @@ func (ms *MutableStateImpl) addBuildIdToLoadedSearchAttribute( // Remove pinned build id search attribute if it exists and we are not pinned if effectiveBehavior != enumspb.VERSIONING_BEHAVIOR_PINNED { + ms.logInfo("removing pinned") newValues = slices.DeleteFunc(newValues, func(s string) bool { return strings.Contains(s, worker_versioning.BuildIdSearchAttributePrefixPinned) }) } + ms.logInfo(fmt.Sprintf("newValues: %+v", newValues)) return newValues } @@ -2776,7 +2786,9 @@ func (ms *MutableStateImpl) addBuildIdToSearchAttributesWithNoVisibilityTask(sta if err != nil { return false, err } + ms.logInfo(fmt.Sprintf("build ids loaded: %+v", existingBuildIds)) modifiedBuildIds := ms.addBuildIdToLoadedSearchAttribute(existingBuildIds, stamp) + ms.logInfo(fmt.Sprintf("build ids modified: %+v", modifiedBuildIds)) if slices.Equal(existingBuildIds, modifiedBuildIds) { return false, nil } @@ -2808,6 +2820,7 @@ func (ms *MutableStateImpl) AddWorkflowTaskCompletedEvent( if err := ms.checkMutability(opTag); err != nil { return nil, err } + ms.logInfo("AddCompleted") return ms.workflowTaskManager.AddWorkflowTaskCompletedEvent(workflowTask, request, limits) } @@ -4283,6 +4296,7 @@ func (ms *MutableStateImpl) AddWorkflowExecutionOptionsUpdatedEvent( return nil, err } event := ms.hBuilder.AddWorkflowExecutionOptionsUpdatedEvent(versioningOverride) + ms.logInfo("UpdateOptions") if err := ms.ApplyWorkflowExecutionOptionsUpdatedEvent(event); err != nil { return nil, err } diff --git a/service/history/workflow/mutable_state_impl_test.go b/service/history/workflow/mutable_state_impl_test.go index c8262992d91..1062f6855b7 100644 --- a/service/history/workflow/mutable_state_impl_test.go +++ b/service/history/workflow/mutable_state_impl_test.go @@ -38,7 +38,6 @@ import ( "go.uber.org/mock/gomock" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/durationpb" - "google.golang.org/protobuf/types/known/fieldmaskpb" "google.golang.org/protobuf/types/known/timestamppb" commandpb "go.temporal.io/api/command/v1" @@ -127,13 +126,6 @@ var ( SeriesName: "my_app", BuildId: "build_3", } - versionOverrideMask = &fieldmaskpb.FieldMask{Paths: []string{"versioning_behavior_override"}} - pinnedOptions1 = &workflowpb.WorkflowExecutionOptions{ - VersioningOverride: &workflowpb.VersioningOverride{ - Behavior: enumspb.VERSIONING_BEHAVIOR_PINNED, - Deployment: deployment1, - }, - } pinnedOptions2 = &workflowpb.WorkflowExecutionOptions{ VersioningOverride: &workflowpb.VersioningOverride{ Behavior: enumspb.VERSIONING_BEHAVIOR_PINNED, @@ -151,12 +143,7 @@ var ( Behavior: enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE, }, } - unspecifiedOptions = &workflowpb.WorkflowExecutionOptions{ - VersioningOverride: &workflowpb.VersioningOverride{ - Behavior: enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, - }, - } - emptyOptions = &workflowpb.WorkflowExecutionOptions{} // should be handled the same as unspecifiedOptions + emptyOptions = &workflowpb.WorkflowExecutionOptions{} ) func TestMutableStateSuite(t *testing.T) { @@ -843,12 +830,17 @@ func (s *mutableStateSuite) verifyOverrides( ) { versioningInfo := s.mutableState.GetExecutionInfo().GetVersioningInfo() s.Equal(expectedBehavior, versioningInfo.Behavior) - s.Equal(expectedBehaviorOverride, versioningInfo.VersioningOverride.Behavior) s.Equal(expectedDeployment, versioningInfo.Deployment) - s.Equal(expectedDeploymentOverride, versioningInfo.VersioningOverride.Deployment) + if versioningInfo.VersioningOverride == nil { + s.Equal(enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, expectedBehaviorOverride) + s.Nil(expectedDeploymentOverride) + } else { + s.Equal(expectedBehaviorOverride, versioningInfo.VersioningOverride.Behavior) + s.Equal(expectedDeploymentOverride, versioningInfo.VersioningOverride.Deployment) + } } -func (s *mutableStateSuite) TestOverride_UnpinnedBase_SetPinnedAndUnsetWithEmptyOptions() { +func (s *mutableStateSuite) TestOverride_UnpinnedBase_SetPinnedAndUnset() { tq := &taskqueuepb.TaskQueue{Name: "tq"} baseBehavior := enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE overrideBehavior := enumspb.VERSIONING_BEHAVIOR_PINNED @@ -869,28 +861,7 @@ func (s *mutableStateSuite) TestOverride_UnpinnedBase_SetPinnedAndUnsetWithEmpty s.verifyOverrides(baseBehavior, enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, deployment1, nil) } -func (s *mutableStateSuite) TestOverride_UnpinnedBase_SetPinnedAndUnsetWithUnspecifiedOptions() { - tq := &taskqueuepb.TaskQueue{Name: "tq"} - baseBehavior := enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE - overrideBehavior := enumspb.VERSIONING_BEHAVIOR_PINNED - s.createMutableStateWithVersioningBehavior(baseBehavior, deployment1, tq) - - // set pinned override - event, err := s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(pinnedOptions2.GetVersioningOverride()) - s.NoError(err) - s.verifyEffectiveDeployment(deployment2, overrideBehavior) - s.verifyWorkflowOptionsUpdatedEvent(event, pinnedOptions2.GetVersioningOverride()) - s.verifyOverrides(baseBehavior, overrideBehavior, deployment1, deployment2) - - // unset pinned override with unspecified - event, err = s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(unspecifiedOptions.GetVersioningOverride()) - s.NoError(err) - s.verifyEffectiveDeployment(deployment1, baseBehavior) - s.verifyWorkflowOptionsUpdatedEvent(event, unspecifiedOptions.GetVersioningOverride()) - s.verifyOverrides(baseBehavior, enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, deployment1, nil) -} - -func (s *mutableStateSuite) TestOverride_PinnedBase_SetUnpinnedAndUnsetWithEmptyOptions() { +func (s *mutableStateSuite) TestOverride_PinnedBase_SetUnpinnedAndUnset() { tq := &taskqueuepb.TaskQueue{Name: "tq"} baseBehavior := enumspb.VERSIONING_BEHAVIOR_PINNED overrideBehavior := enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE @@ -903,7 +874,7 @@ func (s *mutableStateSuite) TestOverride_PinnedBase_SetUnpinnedAndUnsetWithEmpty s.verifyWorkflowOptionsUpdatedEvent(event, unpinnedOptions.GetVersioningOverride()) s.verifyOverrides(baseBehavior, overrideBehavior, deployment1, nil) - // unset pinned override with empty + // unset unpinned override with empty event, err = s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(emptyOptions.GetVersioningOverride()) s.NoError(err) s.verifyEffectiveDeployment(deployment1, baseBehavior) @@ -911,27 +882,6 @@ func (s *mutableStateSuite) TestOverride_PinnedBase_SetUnpinnedAndUnsetWithEmpty s.verifyOverrides(baseBehavior, enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, deployment1, nil) } -func (s *mutableStateSuite) TestOverride_PinnedBase_SetUnpinnedAndUnsetWithUnspecifiedOptions() { - tq := &taskqueuepb.TaskQueue{Name: "tq"} - baseBehavior := enumspb.VERSIONING_BEHAVIOR_PINNED - overrideBehavior := enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE - s.createMutableStateWithVersioningBehavior(baseBehavior, deployment1, tq) - - // set unpinned override - event, err := s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(unpinnedOptions.GetVersioningOverride()) - s.NoError(err) - s.verifyEffectiveDeployment(deployment1, overrideBehavior) - s.verifyWorkflowOptionsUpdatedEvent(event, unpinnedOptions.GetVersioningOverride()) - s.verifyOverrides(baseBehavior, overrideBehavior, deployment1, nil) - - // unset pinned override with unspecified - event, err = s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(unspecifiedOptions.GetVersioningOverride()) - s.NoError(err) - s.verifyEffectiveDeployment(deployment1, baseBehavior) - s.verifyWorkflowOptionsUpdatedEvent(event, unspecifiedOptions.GetVersioningOverride()) - s.verifyOverrides(baseBehavior, enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, deployment1, nil) -} - func (s *mutableStateSuite) TestOverride_RedirectFails() { tq := &taskqueuepb.TaskQueue{Name: "tq"} baseBehavior := enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE @@ -1001,10 +951,10 @@ func (s *mutableStateSuite) TestOverride_BaseDeploymentUpdatedOnCompletion() { s.verifyOverrides(baseBehavior, overrideBehavior, deployment2, deployment3) // now we unset the override and check that the base deployment/behavior is in effect - event, err = s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(unspecifiedOptions.GetVersioningOverride()) + event, err = s.mutableState.AddWorkflowExecutionOptionsUpdatedEvent(emptyOptions.GetVersioningOverride()) s.NoError(err) s.verifyEffectiveDeployment(deployment2, baseBehavior) - s.verifyWorkflowOptionsUpdatedEvent(event, unspecifiedOptions.GetVersioningOverride()) + s.verifyWorkflowOptionsUpdatedEvent(event, emptyOptions.GetVersioningOverride()) s.verifyOverrides(baseBehavior, enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED, deployment2, nil) } From 054b194231e59bf2ea00830d6b6d9ca6099f58d6 Mon Sep 17 00:00:00 2001 From: Carly de Frondeville Date: Wed, 27 Nov 2024 09:11:46 -0800 Subject: [PATCH 2/3] remove logs --- service/history/workflow/mutable_state_impl.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/service/history/workflow/mutable_state_impl.go b/service/history/workflow/mutable_state_impl.go index def60cec901..df8b07d579e 100644 --- a/service/history/workflow/mutable_state_impl.go +++ b/service/history/workflow/mutable_state_impl.go @@ -2407,7 +2407,6 @@ func (ms *MutableStateImpl) AddWorkflowTaskScheduledEvent( if err := ms.checkMutability(opTag); err != nil { return nil, err } - ms.logInfo("AddScheduled") return ms.workflowTaskManager.AddWorkflowTaskScheduledEvent(bypassTaskGeneration, workflowTaskType) } @@ -2454,7 +2453,6 @@ func (ms *MutableStateImpl) AddWorkflowTaskStartedEvent( if err := ms.checkMutability(opTag); err != nil { return nil, nil, err } - ms.logInfo("AddStarted") return ms.workflowTaskManager.AddWorkflowTaskStartedEvent(scheduledEventID, requestID, taskQueue, identity, versioningStamp, redirectInfo, skipVersioningCheck) } @@ -2712,11 +2710,9 @@ func (ms *MutableStateImpl) addBuildIdToLoadedSearchAttribute( ) []string { var newValues []string effectiveBehavior := ms.GetEffectiveVersioningBehavior() - ms.logInfo(fmt.Sprintf("effectiveBehavior: %s", effectiveBehavior.String())) if !stamp.GetUseVersioning() && effectiveBehavior == enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED { // unversioned workflows may still have non-nil deployment, so we don't check deployment newValues = append(newValues, worker_versioning.UnversionedSearchAttribute) } else if effectiveBehavior == enumspb.VERSIONING_BEHAVIOR_PINNED { - ms.logInfo("appended pinned SA") newValues = append(newValues, worker_versioning.PinnedBuildIdSearchAttribute(ms.getPinnedDeployment())) } else if ms.GetAssignedBuildId() != "" { newValues = append(newValues, worker_versioning.AssignedBuildIdSearchAttribute(ms.GetAssignedBuildId())) @@ -2740,12 +2736,10 @@ func (ms *MutableStateImpl) addBuildIdToLoadedSearchAttribute( // Remove pinned build id search attribute if it exists and we are not pinned if effectiveBehavior != enumspb.VERSIONING_BEHAVIOR_PINNED { - ms.logInfo("removing pinned") newValues = slices.DeleteFunc(newValues, func(s string) bool { return strings.Contains(s, worker_versioning.BuildIdSearchAttributePrefixPinned) }) } - ms.logInfo(fmt.Sprintf("newValues: %+v", newValues)) return newValues } @@ -2786,9 +2780,7 @@ func (ms *MutableStateImpl) addBuildIdToSearchAttributesWithNoVisibilityTask(sta if err != nil { return false, err } - ms.logInfo(fmt.Sprintf("build ids loaded: %+v", existingBuildIds)) modifiedBuildIds := ms.addBuildIdToLoadedSearchAttribute(existingBuildIds, stamp) - ms.logInfo(fmt.Sprintf("build ids modified: %+v", modifiedBuildIds)) if slices.Equal(existingBuildIds, modifiedBuildIds) { return false, nil } @@ -2820,7 +2812,6 @@ func (ms *MutableStateImpl) AddWorkflowTaskCompletedEvent( if err := ms.checkMutability(opTag); err != nil { return nil, err } - ms.logInfo("AddCompleted") return ms.workflowTaskManager.AddWorkflowTaskCompletedEvent(workflowTask, request, limits) } From 5a5fdb19f50c8ec3f2691dbec589967595bcc8ca Mon Sep 17 00:00:00 2001 From: Carly de Frondeville Date: Wed, 27 Nov 2024 09:12:35 -0800 Subject: [PATCH 3/3] remove log --- service/history/workflow/mutable_state_impl.go | 1 - 1 file changed, 1 deletion(-) diff --git a/service/history/workflow/mutable_state_impl.go b/service/history/workflow/mutable_state_impl.go index df8b07d579e..8958c6e9683 100644 --- a/service/history/workflow/mutable_state_impl.go +++ b/service/history/workflow/mutable_state_impl.go @@ -4287,7 +4287,6 @@ func (ms *MutableStateImpl) AddWorkflowExecutionOptionsUpdatedEvent( return nil, err } event := ms.hBuilder.AddWorkflowExecutionOptionsUpdatedEvent(versioningOverride) - ms.logInfo("UpdateOptions") if err := ms.ApplyWorkflowExecutionOptionsUpdatedEvent(event); err != nil { return nil, err }