-
Notifications
You must be signed in to change notification settings - Fork 877
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update backfill history in sync workflow state #3042
Conversation
return &persistence.PutReplicationTaskToDLQRequest{ | ||
ShardID: p.shard.GetShardID(), | ||
SourceClusterName: p.sourceCluster, | ||
TaskInfo: &persistencespb.ReplicationTaskInfo{ |
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.
I kinda wonder if we should generate a tasks.SyncWorkflowStateTask
here and let persistence layer do the conversion to proto representation. Similar to how normal replication tasks are generated & persisted.
If we were to have DLQ for other queues in the future, that should help unify the code path.
latestBranchID := historyBranch.GetBranchId() | ||
var prevBranchID string | ||
|
||
sortedAncestors := copyAncestors(historyBranch.GetAncestors()) |
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.
Why do you copy and then substitute original historyBranch
instead of just sorting ancestors in place and then create new filteredHistoryBranch
and fill in the loop bellow?
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.
This is easier to achieve. For example, there are 3 branches. B1 has no ancestor. B2 has B1 ancestor, B3 has B1,B2 ancestors.
B1
B2
/
B3
If create new filteredHistoryBranch in the loop, then I need to fill in the previous ancestors.
* Update backfill history in sync workflow state * append node with cleanup info * get last batch node id
What changed?
Why?
Correctness issue
How did you test it?
New unit tests and functional tests.
Potential risks
Is hotfix candidate?
Yes.