Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary aliasing/unaliasing of scheduled workflow search attributes #3943

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Remove unnecessary aliasing/unaliasing of scheduled workflow search attributes #3943

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 10 additions & 67 deletions service/frontend/workflow_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2966,6 +2966,9 @@ func (wh *WorkflowHandler) CreateSchedule(ctx context.Context, request *workflow
return nil, err
}

// Add namespace division before unaliasing search attributes.
searchattribute.AddSearchAttribute(&request.SearchAttributes, searchattribute.TemporalNamespaceDivision, payload.EncodeString(scheduler.NamespaceDivision))

request, err = wh.unaliasCreateScheduleRequestSearchAttributes(request, namespaceName)
if err != nil {
return nil, err
Expand Down Expand Up @@ -3001,8 +3004,6 @@ func (wh *WorkflowHandler) CreateSchedule(ctx context.Context, request *workflow
}
// Add initial memo for list schedules
wh.addInitialScheduleMemo(request, input)
// Add namespace division
searchattribute.AddSearchAttribute(&request.SearchAttributes, searchattribute.TemporalNamespaceDivision, payload.EncodeString(scheduler.NamespaceDivision))
// Create StartWorkflowExecutionRequest
startReq := &workflowservice.StartWorkflowExecutionRequest{
Namespace: request.Namespace,
Expand Down Expand Up @@ -3070,7 +3071,12 @@ func (wh *WorkflowHandler) validateStartWorkflowArgsForSchedule(
return errIDReusePolicyNotAllowed
}

if err := wh.validateSearchAttributes(startWorkflow.GetSearchAttributes(), namespaceName); err != nil {
// Unalias startWorkflow search attributes only for validation. Keep aliases in request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Unalias startWorkflow search attributes only for validation. Keep aliases in request.
// Unalias startWorkflow search attributes only for validation. Keep aliases in the request, because the request will be sent back to frontend to start workflows, which will unalias at that point.

unaliasedStartWorkflowSas, err := searchattribute.UnaliasFields(wh.saMapperProvider, startWorkflow.GetSearchAttributes(), namespaceName.String())
if err != nil {
return err
}
if err := wh.validateSearchAttributes(unaliasedStartWorkflowSas, namespaceName); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't like bellow lint message. I always do explicit error return. Because what it suggests looks like the function returns results of another function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this contradicts the standard Go style of having the error path exit early and the happy path flow to the end of the function. Sometimes for short functions I might return a call directly but if there are other error returns I agree the last one should also be an error return for consistency

return err
}

Expand Down Expand Up @@ -3164,22 +3170,6 @@ func (wh *WorkflowHandler) DescribeSchedule(ctx context.Context, request *workfl
return err
}

// map action search attributes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just replace with a comment like

// Search attributes in the Action are already in external ("aliased") form. Do not alias them here.

if sa := queryResponse.Schedule.Action.GetStartWorkflow().SearchAttributes; sa != nil {
saTypeMap, err := wh.saProvider.GetSearchAttributes(wh.visibilityMrg.GetIndexName(), false)
if err != nil {
return serviceerror.NewUnavailable(fmt.Sprintf(errUnableToGetSearchAttributesMessage, err))
}
searchattribute.ApplyTypeMap(sa, saTypeMap)
aliasedSas, err := searchattribute.AliasFields(wh.saMapperProvider, sa, request.Namespace)
if err != nil {
return err
}
if aliasedSas != nil {
queryResponse.Schedule.Action.GetStartWorkflow().SearchAttributes = aliasedSas
}
}

// for all running workflows started by the schedule, we should check that they're
// still running, and if not, poke the schedule to refresh
needRefresh := false
Expand Down Expand Up @@ -3313,11 +3303,6 @@ func (wh *WorkflowHandler) UpdateSchedule(ctx context.Context, request *workflow
return nil, err
}

request, err = wh.unaliasUpdateScheduleRequestStartWorkflowSearchAttributes(request, namespaceName)
if err != nil {
return nil, err
}

input := &schedspb.FullUpdateRequest{
Schedule: request.Schedule,
}
Expand Down Expand Up @@ -4960,13 +4945,7 @@ func (wh *WorkflowHandler) unaliasCreateScheduleRequestSearchAttributes(request
return nil, err
}

startWorkflow := request.GetSchedule().GetAction().GetStartWorkflow()
unaliasedStartWorkflowSas, err := searchattribute.UnaliasFields(wh.saMapperProvider, startWorkflow.GetSearchAttributes(), namespaceName.String())
if err != nil {
return nil, err
}

if unaliasedSas == nil && unaliasedStartWorkflowSas == nil {
if unaliasedSas == nil {
return request, nil
}

Expand All @@ -4977,41 +4956,5 @@ func (wh *WorkflowHandler) unaliasCreateScheduleRequestSearchAttributes(request
newRequest.SearchAttributes = unaliasedSas
}

if unaliasedStartWorkflowSas != nil && startWorkflow != nil {
newStartWorkflow := *startWorkflow
newStartWorkflow.SearchAttributes = unaliasedStartWorkflowSas
newSchedule := *request.GetSchedule()
newSchedule.Action = &schedpb.ScheduleAction{
Action: &schedpb.ScheduleAction_StartWorkflow{
StartWorkflow: &newStartWorkflow,
}}
newRequest.Schedule = &newSchedule
}

return &newRequest, nil
}

func (wh *WorkflowHandler) unaliasUpdateScheduleRequestStartWorkflowSearchAttributes(request *workflowservice.UpdateScheduleRequest, namespaceName namespace.Name) (*workflowservice.UpdateScheduleRequest, error) {
startWorkflow := request.GetSchedule().GetAction().GetStartWorkflow()
if startWorkflow == nil {
return request, nil
}

unaliasedSas, err := searchattribute.UnaliasFields(wh.saMapperProvider, startWorkflow.GetSearchAttributes(), namespaceName.String())
if err != nil {
return nil, err
}
if unaliasedSas == nil {
return request, nil
}
newStartWorkflow := *startWorkflow
newStartWorkflow.SearchAttributes = unaliasedSas
newSchedule := *request.GetSchedule()
newSchedule.Action = &schedpb.ScheduleAction{
Action: &schedpb.ScheduleAction_StartWorkflow{
StartWorkflow: &newStartWorkflow,
}}
newRequest := *request
newRequest.Schedule = &newSchedule
return &newRequest, nil
}