Handle applyParentClose target domain failover #4533
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
demirkayaender
force-pushed
the
applyParent
branch
from
3e735a6 to
b8f0af8
Compare
yycptt
reviewed
Oct 12, 2021
| @@ -585,6 +586,7 @@ type ( | |||
|
|
|||
| // ApplyParentClosePolicyTask identifies a task for applying parent close policy | |||
| ApplyParentClosePolicyTask struct { | |||
| DomainList []string | |||
There was a problem hiding this comment.
Do you plan to also add this field to CloseExecutionTransfer task? If some children's domains are active in the current cluster, then we can record them and only process child workflows from those domains when executing the transfer task.
There was a problem hiding this comment.
Yeah, that will be in a separate PR, right now I'm just fixing the failover issue we talked about so source won't skip any child.
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go
Outdated
Show resolved
Hide resolved
demirkayaender
force-pushed
the
applyParent
branch
4 times, most recently
from
aaf7aae to
ec8173f
Compare
yycptt
reviewed
Oct 14, 2021
service/history/task/cross_cluster_source_task_executor_test.go
Outdated
Show resolved
Hide resolved
yycptt
reviewed
Oct 15, 2021
demirkayaender
force-pushed
the
applyParent
branch
from
ec8173f to
2e6965b
Compare
demirkayaender
force-pushed
the
applyParent
branch
from
2e6965b to
6664fe6
Compare
yycptt
approved these changes
Oct 15, 2021
demirkayaender
force-pushed
the
applyParent
branch
from
3e9401e to
9fd179a
Compare
demirkayaender
force-pushed
the
applyParent
branch
from
9fd179a to
0caef5f
Compare
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?
With the previous implementation, there was a race condition where we could skip sending parentClosePolicy to one of the cross cluster children if that child was failing over at the same time. This diff ensures that we will track all the children and retry applyParentClosePolicy task if this happens.
In a later diff, I'm planning to split processCloseExecution into 3 functions as
processCloseExecution,recordChildCompletionandapplyParentPolicy; then, planning to create 2 new transfer tasks for the 2 new later functions. This will help us retry only the failed parts of closeExecutions instead of retrying the whole thing. processCloseExecution will still call the other 2 as it's doing today.Depends on cadence-workflow/cadence-idl#89
Why?
Without this diff, we might leak some child workflows or have unintended child workflows running.
How did you test it?
Unit tests.
Potential risks
Release notes
Documentation Changes