Support independent activities for pinned workflows #6957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ivity # Conflicts: # service/matching/task_queue_partition_manager.go
dnr
reviewed
Dec 10, 2024
| } | ||
| // Finish the task because now it is copied to the other backlog. It should be considered | ||
| // invalid because a poller did not receive the task. | ||
| task.finish(nil, false) |
There was a problem hiding this comment.
should we return after this line? or continue to try to dispatch on syncMatchQueue?
| wfBehavior := directive.GetBehavior() | ||
| deployment := directive.GetDeployment() | ||
|
|
||
| perTypeUserData, perTypeUserDataChanged, err := pm.getPerTypeUserData() |
There was a problem hiding this comment.
I would still call it
Suggested change
| perTypeUserData, perTypeUserDataChanged, err := pm.getPerTypeUserData() | |
| perTypeUserData, userDataChanged, err := pm.getPerTypeUserData() |
the channel broadcast is still when any user data changes, right?
| @@ -172,13 +172,23 @@ func recordActivityTaskStarted( | |||
| // TODO (shahab): support independent activities. Independent activities do not need all | |||
| if scheduledDeployment == nil { | ||
| // TODO: remove this once the ScheduledDeployment field is removed from proto | ||
| scheduledDeployment = req.GetScheduledDeployment() | ||
| } | ||
| if !scheduledDeployment.Equal(wfDeployment) { | ||
| // This must be an AT scheduled before the workflow transitions to the current |
| // we skip this check until a v1.28 if scheduledBehavior is unspecified. | ||
| // TODO (shahab): remove this line after v1.27 is released. | ||
| scheduledBehavior != enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED { | ||
| // This must be an AT scheduled before the workflow changes behavior. Matching can drop it. |
| // TODO (shahab): remove this line after v1.27 is released. | ||
| scheduledBehavior != enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED { | ||
| // This must be an AT scheduled before the workflow changes behavior. Matching can drop it. | ||
| return nil, false, serviceerrors.NewObsoleteMatchingTask("wrong directive behavior") |
There was a problem hiding this comment.
could the error message be a little more descriptive? "wrong directive behavior" and "wrong directive deployment" are pretty obscure
| @@ -167,10 +167,24 @@ func Invoke( | |||
| return nil, serviceerrors.NewObsoleteMatchingTask("wrong task queue type") | |||
| } | |||
|
|
|||
| wfBehavior := mutableState.GetEffectiveVersioningBehavior() | |||
There was a problem hiding this comment.
is there any opportunity to share code between this and recordactivitytaskstarted? actually I'm not even sure that would make this easier to understand, maybe it's simpler to just do it inline in both. (especially after future cleanups.) just wondering.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Now, a pinned workflow can start activities in task queues belonging to different deployments. Such activities are versioned independently from the workflow and will start on their own task queue's current deployment.
Why?
Previously, independent activities for pinned workflows were not supported and the workflow would get stuck if attempting to start an activity on a task queue not belonging to the same deployment and the workflow task queue.
How did you test it?
Added test.
Potential risks
The independence check happens when the task is scheduled and then when the spooled task is being dispatched. If the independence check have a different result for the spooled task, we re-spool it in the new backlog.
Re-spooling tasks in a different queue is a new behavior, but since it should happen at most once for a task (from default queue to pinned queue) and only in rare edge cases dealing with late arriving pollers, it seems safe.
Documentation
None so far.
Is hotfix candidate?
No.