-
Notifications
You must be signed in to change notification settings - Fork 266
[DRAFT/PROPOSAL] continue-as-new-suggested reason and signal #2082
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1199,6 +1199,9 @@ func (weh *workflowExecutionEventHandlerImpl) ProcessEvent( | |
| // Update workflow info fields | ||
| weh.workflowInfo.currentHistoryLength = int(event.EventId) | ||
| weh.workflowInfo.continueAsNewSuggested = event.GetWorkflowTaskStartedEventAttributes().GetSuggestContinueAsNew() | ||
| weh.workflowInfo.continueAsNewSuggestedReason = convertContinueAsNewSuggestedReason( | ||
| event.GetWorkflowTaskStartedEventAttributes().GetContinueAsNewSuggestedReason(), | ||
| ) | ||
| weh.workflowInfo.currentHistorySize = int(event.GetWorkflowTaskStartedEventAttributes().GetHistorySizeBytes()) | ||
| // Reset the counter on command helper used for generating ID for commands | ||
| weh.commandsHelper.setCurrentWorkflowTaskStartedEventID(event.GetEventId()) | ||
|
|
@@ -1751,9 +1754,35 @@ func (weh *workflowExecutionEventHandlerImpl) ProcessLocalActivityResult(lar *lo | |
| func (weh *workflowExecutionEventHandlerImpl) handleWorkflowExecutionSignaled( | ||
| attributes *historypb.WorkflowExecutionSignaledEventAttributes, | ||
| ) error { | ||
| switch attributes.GetSignalName() { | ||
| case SignalTypeSuggestContinueAsNew: | ||
| weh.workflowInfo.continueAsNewSuggested = true | ||
| var reason enumspb.ContinueAsNewSuggestedReason | ||
| err := weh.dataConverter.FromPayload(attributes.GetInput().GetPayloads()[0], &reason) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Signal Handling Accesses Unchecked PayloadIn |
||
| if err != nil { | ||
| return fmt.Errorf("unable to convert continue-as-new suggested reason: %w", err) | ||
| } | ||
| weh.workflowInfo.continueAsNewSuggestedReason = convertContinueAsNewSuggestedReason(reason) | ||
| } | ||
| return weh.signalHandler(attributes.GetSignalName(), attributes.Input, attributes.Header) | ||
| } | ||
|
|
||
| // TODO(carlydf): move this helper somewhere appropriate | ||
| func convertContinueAsNewSuggestedReason(reason enumspb.ContinueAsNewSuggestedReason) ContinueAsNewSuggestedReason { | ||
| switch reason { | ||
| case enumspb.CONTINUE_AS_NEW_SUGGESTED_REASON_UNSPECIFIED: | ||
| return ContinueAsNewSuggestedReasonUnspecified | ||
| case enumspb.CONTINUE_AS_NEW_SUGGESTED_REASON_UPDATE_REGISTRY_TOO_LARGE: | ||
| return ContinueAsNewSuggestedReasonUpdateRegistryTooLarge | ||
| case enumspb.CONTINUE_AS_NEW_SUGGESTED_REASON_HISTORY_SIZE_TOO_LARGE: | ||
| return ContinueAsNewSuggestedReasonHistorySizeTooLarge | ||
| case enumspb.CONTINUE_AS_NEW_SUGGESTED_REASON_WORKER_DEPLOYMENT_VERSION_CHANGED: | ||
| return ContinueAsNewSuggestedReasonWorkerDeploymentVersionChanged | ||
| default: | ||
| return ContinueAsNewSuggestedReasonUnspecified | ||
| } | ||
| } | ||
|
|
||
| func (weh *workflowExecutionEventHandlerImpl) handleStartChildWorkflowExecutionFailed(event *historypb.HistoryEvent) error { | ||
| attributes := event.GetStartChildWorkflowExecutionFailedEventAttributes() | ||
| childWorkflowID := attributes.GetWorkflowId() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a proposal to change every SDK to accept a special built-in signal name to suggest continue as new? I would recommend keeping the one canonical place to get suggest-continue-as-new, in the task event. And if we need an RPC call to have the server apply this manually instead of how it does today, we can add it I think. Is the whole reason to use a signal because 1) we're concerned we can't send an empty task, and 2) we're unwilling to make a continue-as-new-suggested event?
If we want to take an approach like this, I think we can make continue as new suggested something that can be updated via
UpdateWorkflowExecutionOptions(or separate RPC) and useWorkflowPropertiesModifiedExternallyEventAttributesorWorkflowExecutionOptionsUpdatedEventAttributesto relay the update to the SDK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Chad, my intention was not for this to be ready for review as an actual ready-to-merge PR (it doesn't even have tests or a server implementation for the API change yet). I'm working on a blueprint for the
PINNED_UNTIL_CONTINUE_AS_NEWbehavior / "Trampolining" feature and wanted to use this PR diff as a way to communicate my initial ideas for how it could be implemented in the SDKs.I don't intend to skip cross-SDK design for this at all. In fact, this PR is still in the design phase. Sketching it out in Go felt like the most straightforward way to get a sense of whether the implementation was technically viable, since I'm not fluent enough in any of our other SDKs to sketch it there.
I thought marking it as DRAFT/PROPOSAL while I worked on some other things would sufficiently communicate that it wasn't ready for review, but next time I'll be more explicit in the PR description as well. Keep an eye out for an incoming https://github.com/temporalio/features/issues and request for comment on my blueprint!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason to use a signal is to provide an API through which inactive workflows can be woken up so that they have the opportunity to continue-as-new onto a new worker version even if they are waiting on a timer or other condition.
Ideally this suggestion signal / empty task / API could be exposed to users so that they can apply this suggestion to workflows when the user's specific validation requirements are met for the new version if they choose.
Signal seems like a straightforward way to do that, but
WorkflowExecutionOptionsUpdatedEventAttributes+ scheduling an empty task could totally do the same thing, and I think is a great option that I hadn't thought of!