Decouple StateBuilder from TaskGenerator #4991
Merged
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
vytautas-karpavicius
force-pushed
the
decouple-state-builder
branch
from
3b2d81d
to
2d93890
Compare
Shaddoll
reviewed
Sep 14, 2022
| @@ -2174,12 +2166,6 @@ func (e *mutableStateBuilder) AddActivityTaskScheduledEvent( | |||
| activityStartedScope.IncCounter(metrics.CadenceRequests) | |||
| return event, ai, nil, true, true, nil | |||
| } | |||
| // TODO merge active & passive task generation | |||
There was a problem hiding this comment.
This will change the behavior of activity local dispatch.
Before, we try dispatch activity task and if it succeeds, we skip the transfer task creation.
After, we always create the transfer task.
There was a problem hiding this comment.
Yes, updated. Good catch.
Shaddoll
reviewed
Sep 14, 2022
| @@ -4690,8 +4628,7 @@ func (e *mutableStateBuilder) closeTransactionHandleActivityUserTimerTasks( | |||
| transactionPolicy TransactionPolicy, | |||
There was a problem hiding this comment.
It seems that we can remove this parameter
There was a problem hiding this comment.
Thanks, updated.
Shaddoll
approved these changes
Nov 7, 2022
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?
Decoupled
StateBuilderfromTaskGeneratorby moving task generation toReplicate<...>functions on mutable state. Task generation is needed on both active and passive side, so there is no need to call them separately. There were only a few places where active/passive side differs slightly. For them we can simply pass additional flag modifying that behavior. In particular, more attention is needed for:ReplicateWorkflowExecutionStartedEventwillGenerateDelayedDecisionTasksonly for passive side. Active side does that separately duringAddFirstDecisionTaskScheduled.GenerateActivityTimerTasks/GenerateUserTimerTaskswas called. This can be skipped here, and instead call when closing a transaction. Previously it was not done due totransactionPolicy == TransactionPolicyPassive, now this is removed. And they will be generated for both active/passive cases.Why?
// TODO merge active & passive task generation.How did you test it?
Existing tests
Potential risks
Release notes
Documentation Changes