Fix reuse + conflict policy validation#9728
Conversation
| ) | ||
| } | ||
|
|
||
| func MigrateWorkflowIdReusePolicyForRunningWorkflow( |
There was a problem hiding this comment.
Moved this into SetDefaultWorkflowIdPolicies
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
from
c4dd8c4 to
3260462
Compare
| enumspb "go.temporal.io/api/enums/v1" | ||
| ) | ||
|
|
||
| func SetDefaultWorkflowIdReusePolicy(f *enumspb.WorkflowIdReusePolicy) { |
There was a problem hiding this comment.
Still used elsewhere
There was a problem hiding this comment.
should it be used elsewhere by itself? or should we always use a function that handles these two together?
There was a problem hiding this comment.
it's used in handleCommandStartChildWorkflow and child workflows don't have a conflict policy
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
4 times, most recently
from
0dd2220 to
2b67de7
Compare
|
|
||
| func SetDefaultWorkflowIdConflictPolicy( | ||
| // SetDefaultWorkflowIdPolicies migrates and applies defaults to both WorkflowId policies. | ||
| func SetDefaultWorkflowIdPolicies( |
There was a problem hiding this comment.
One to rule them all
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
2 times, most recently
from
3bf57ef to
98b7061
Compare
stephanos
changed the title
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
from
98b7061 to
e76f4d6
Compare
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
from
e76f4d6 to
97fa22b
Compare
| if *reusePolicy == enumspb.WORKFLOW_ID_REUSE_POLICY_TERMINATE_IF_RUNNING { | ||
| // Migrate the deprecated TERMINATE_IF_RUNNING. | ||
| *conflictPolicy = enumspb.WORKFLOW_ID_CONFLICT_POLICY_TERMINATE_EXISTING | ||
| *reusePolicy = enumspb.WORKFLOW_ID_REUSE_POLICY_ALLOW_DUPLICATE |
There was a problem hiding this comment.
Same default as in MigrateWorkflowIdReusePolicyForRunningWorkflow
| enumspb "go.temporal.io/api/enums/v1" | ||
| ) | ||
|
|
||
| func SetDefaultWorkflowIdReusePolicy(f *enumspb.WorkflowIdReusePolicy) { |
There was a problem hiding this comment.
should it be used elsewhere by itself? or should we always use a function that handles these two together?
| enumspb.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING) | ||
|
|
||
| api.MigrateWorkflowIdReusePolicyForRunningWorkflow( | ||
| enums.SetDefaultWorkflowIDPolicies( |
There was a problem hiding this comment.
should this be moved higher near the start of this function, like ones in workflow_handler.go and startworkflow/api.go? it's above "validation" here but could be earlier.
also, if we do this in frontend, why do we need to do it here?
There was a problem hiding this comment.
also, if we do this in frontend, why do we need to do it here?
I'll answer in https://github.com/temporalio/temporal/pull/9728/changes#r3030624859
| return nil, err | ||
| } | ||
|
|
||
| // TODO: remove this call in 1.25 |
There was a problem hiding this comment.
is the todo wrong? should we remove the call now (long past 1.25)?
There was a problem hiding this comment.
I considered removing the calls because IIRC this was only needed for the "mixed brain" scenario (what the comment implies). When I put the PR together I wasn't quite sure and opted to leave it - but thinking about it more, unless you can think of a reason to keep it, it seems safe to remove altogether (same in service/history/api/startworkflow/api.go). I'll do that.
stephanos
force-pushed
the
stephanos/fix-conflict-policy-validation
branch
from
5d53822 to
c03bd51
Compare
| @@ -120,15 +119,6 @@ func NewStarter( | |||
| func (s *Starter) prepare(ctx context.Context) error { | |||
| request := s.request.StartRequest | |||
|
|
|||
| // TODO: remove this call in 1.25 | |||
There was a problem hiding this comment.
Dropping these now since they were only needed in-between versions but now frontend ensures policies are correct
## What changed? Fix setting of default reuse + conflict policy. ## Why? Currently when the frontend request is retried, the request can suddenly be in a bad state that doesn't pass validation if the conflict policy was originally unset and the reuse policy was terminate-if-running because setting the defaults will set a conflict policy - which is not allowed when reuse policy terminate-if-running is used. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s)
2 participants
What changed?
Fix setting of default reuse + conflict policy.
Why?
Currently when the frontend request is retried, the request can suddenly be in a bad state that doesn't pass validation if the conflict policy was originally unset and the reuse policy was terminate-if-running because setting the defaults will set a conflict policy - which is not allowed when reuse policy terminate-if-running is used.
How did you test it?