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.

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

Activity local dispatch #2618

Merged
merged 5 commits into from
Mar 22, 2022
Merged

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented Mar 18, 2022

What changed?

  • Support activity local dispatch: an optimization for skipping matching and task polling for running activity tasks.
  • When responding workflow task complete, sdk can set request_start to true for an activity command. If server accepts (currently server always accepts if the feature is enabled for that namespace) the request, then the activity will be scheduled AND started, and be returned to the sdk in respondWorkflowTaskResponse.
  • SDK should guarantee that if request_start is true, it's has the capacity and is able to run the activity (even if it's cross-namespace or on a different task list)
  • Server will guarantee only those activities requested to be started will be included in respondWorkflowTaskResponse.
  • API change: Activity local dispatch api#159

Why?

  • Activity local dispatch

How did you test it?

  • Added unit test

Potential risks

  • No, feature disabled by default.

Is hotfix candidate?

  • No.

return nil, nil
}

if _, err := handler.mutableState.AddActivityTaskStartedEvent(
Copy link
Member

Choose a reason for hiding this comment

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

does server control how many activities are allowed for local dispatch? or all of them as SDK asked?

Copy link
Member Author

Choose a reason for hiding this comment

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

Synced offline. Currently there's no obvious reason for server to reject the request (even if the activity is scheduled cross namespace or on a different task queue). The workflow mutable state update transaction and history have the same size.

If worker set request_start to true, then it should guarantee it is able to run that activity.

service/history/workflowTaskHandler.go Outdated Show resolved Hide resolved
Comment on lines 320 to 321
// RetryPolicy: TODO: do we need this field?
// not specified by matching or frontend as well
Copy link
Member

Choose a reason for hiding this comment

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

it seems this is not used by sdk (checked sdk-go). I think we should not have this field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the TODO. I will double check with SDK side before removing it from proto def.

@yycptt yycptt marked this pull request as ready for review March 22, 2022 23:09
@yycptt yycptt requested a review from a team as a code owner March 22, 2022 23:09
@yycptt yycptt merged commit 7fb1bbe into temporalio:master Mar 22, 2022
@yycptt yycptt deleted the activity-local-dispatch branch March 22, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants