Update replication use branch token #3447
Conversation
| taskInfo.FirstEventID, | ||
| taskInfo.NextEventID, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var newRunBranchToken []byte | ||
| if len(taskInfo.NewRunID) > 0 { | ||
| newRunContext, releaseFn, err := p.workflowCache.GetOrCreateWorkflowExecution( |
There was a problem hiding this comment.
if new run ID is set, then here is taking another lock.
can we modify the logic to lock & get the branch token one by one, instead of double lock & double unlock?
There was a problem hiding this comment.
I see the concern here. Updated to not have the overlap.
service/history/workflow/context.go
Outdated
| newRunBranchToken := newRunTask.BranchToken | ||
| newRunID := newRunTask.RunID | ||
| taskUpdated := false | ||
| for _, replicationTask := range currentWorkflowMutation.Tasks[tasks.CategoryReplication] { | ||
| if task, ok := replicationTask.(*tasks.HistoryReplicationTask); ok { | ||
| taskUpdated = true | ||
| task.NewRunBranchToken = newRunBranchToken | ||
| task.NewRunID = newRunID |
There was a problem hiding this comment.
to support forward / backward compatibility, branch token & run ID should both be set?
task processing logic should use run ID if run ID is set?
There was a problem hiding this comment.
Good catch. Keep both for compatibility
| taskVersion, | ||
| ) | ||
| if err != nil { | ||
| releaseFn(err) |
There was a problem hiding this comment.
if the intention is to reduce locking, then maybe create a function just for getting the branch token? then reuse this function for normal runs & continue as new runs
* Update replication use branch token
What changed?
Why?
How did you test it?
Potential risks
Is hotfix candidate?