Support updating closed workflows by mutation #7420
Conversation
yycptt
changed the title
|
LGTM. No concern on this change. |
yycptt
force-pushed
the
update-skip-current
branch
from
0ae5403 to
8e018ba
Compare
yycptt
changed the title
yycptt
force-pushed
the
update-skip-current
branch
3 times, most recently
from
bc9e82b to
6169664
Compare
yycptt
force-pushed
the
update-skip-current
branch
from
6169664 to
e2ab6b4
Compare
| return executionContext.UpdateWorkflowExecutionAsPassive(ctx, r.shardContext) | ||
| } | ||
|
|
||
| // TODO: remove following code once EnableUpdateClosedWorkflowByMutation config is deprecated. | ||
| updateMode := persistence.UpdateWorkflowModeUpdateCurrent | ||
| if state, _ := mutableState.GetWorkflowStateStatus(); state == enumsspb.WORKFLOW_EXECUTION_STATE_ZOMBIE { |
There was a problem hiding this comment.
The previous logic is kinda wrong/misleading as it making decision based on the in memory workflow state instead of the workflow state in DB. The updateMode though, is for doing current record CAS in db, so we need to pick it based on the state in db (i.e. the workflow state before the current mutable state transaction)
The logic still works because activity state replicator (and same for the hsm state replicator) below don't really change the workflow state, so in memory state == state in DB.
| return wfCtx.UpdateWorkflowExecutionAsActive(ctx, e.shardContext) | ||
| } | ||
|
|
||
| // TODO: remove following code once EnableUpdateClosedWorkflowByMutation config is deprecated. | ||
| if ms.GetExecutionState().State == enumsspb.WORKFLOW_EXECUTION_STATE_COMPLETED { |
There was a problem hiding this comment.
Same here. The hsm framework doesn't really change workflow state right now.
common/dynamicconfig/constants.go
Outdated
| @@ -1425,6 +1425,12 @@ HistoryCacheSizeBasedLimit is set to true.`, | |||
| timeout timer when execution timeout is specified when starting a workflow. | |||
| For backward compatibility, this feature is disabled by default and should only be enabled after server version | |||
| containing this flag is deployed to all history service nodes in the cluster.`, | |||
| ) | |||
| EnableUpdateClosedWorkflowByMutation = NewGlobalBoolSetting( | |||
There was a problem hiding this comment.
Nit: Name this EnableUpdateWorkflowModeSkipCurrent or something similar since that is what it enables. It makes it easier to discover this config and its purpose.
| // For example, when updating a closed workflow, it may or may not be the current workflow. | ||
| // This is similar to SetWorkflowExecution, but UpdateWorkflowExecution with this mode persists the workflow as a mutation, | ||
| // instead of a snapshot. | ||
| UpdateWorkflowModeSkipCurrent |
There was a problem hiding this comment.
This does conflict a lot with UpdateWorkflowModeBypassCurrent.
Can we consider something like UpdateWorkflowModeIgnoreCurrent? It'll be even better if we can get rid of one of these modes all together (if possible)
There was a problem hiding this comment.
yeah makes sense. IgnoreCurrent is a better name.
This updateMode is part of the persistence interface so not trivial to get rid of any existing ones.
Long term if we ever get a chance to redo our persistence interface, I think the interface can explicit ask to the caller to state which columns/rows to update and to what, including changes for current rows. That will make things much more explicit. Right now the current row concept, which is an application concept is almost entirely in persistence layer...
| @@ -461,6 +461,12 @@ func (t *visibilityQueueTaskExecutor) cleanupExecutionInfo( | |||
| executionInfo.Memo = nil | |||
| executionInfo.SearchAttributes = nil | |||
| executionInfo.RelocatableAttributesRemoved = true | |||
|
|
|||
| if t.shardContext.GetConfig().EnableUpdateClosedWorkflowByMutation() { | |||
| return weContext.UpdateWorkflowExecutionAsPassive(ctx, t.shardContext) | |||
There was a problem hiding this comment.
Curious why UpdateWorkflowExecutionAsPassive here and HSM state replicator. Why not UpdateWorkflowExecutionAsActive?
There was a problem hiding this comment.
HSM/Activity state replicator are replication logic for applying changes in the standby cluster of a namespace. And when "AsPassive" is used, the mutable state logic will prevent another replication task from being generated (there are some other differences, but this is the most important one).
For visibility, you can think this the removal of Memo & SearchAttributes is a cluster local operation (move the data from mutable state to ES). and this operation doesn't need to be replicated.
What changed?
Why?
SetWorkflowExecutionpersistence API in order to skip the current record validation.How did you test it?
Potential risks
Documentation
Is hotfix candidate?