Deny ReusePolicy RejectDuplicate for ConflictPolicy TerminateExisting #7099
Conversation
common/dynamicconfig/constants.go
Outdated
| AllowReusePolicyRejectWithConflictPolicyTerminate = NewNamespaceTypedSetting( | ||
| "history.allowReusePolicyRejectWithConflictPolicyTerminate", | ||
| false, | ||
| `Allows to use WorkflowIdReusePolicy RejectDuplicate with WorkflowIdReusePolicy TerminateExisting. | ||
| If false, an invalid argument error is returned.`, |
There was a problem hiding this comment.
nit: Might be worth having a single flag for this and changing the behavior of RejectDuplicateFailedOnly.
I'd also consider adding a warning that this flag may go away in upcoming releases, I don't think we want to keep this forever.
| AllowReusePolicyRejectWithConflictPolicyTerminate = NewNamespaceTypedSetting( | |
| "history.allowReusePolicyRejectWithConflictPolicyTerminate", | |
| false, | |
| `Allows to use WorkflowIdReusePolicy RejectDuplicate with WorkflowIdReusePolicy TerminateExisting. | |
| If false, an invalid argument error is returned.`, | |
| AllowReusePolicyRejectWithConflictPolicyTerminate = NewNamespaceTypedSetting( | |
| "history.allowReusePolicyRejectWithConflictPolicyTerminate", | |
| false, | |
| `Allows to use WorkflowIdReusePolicy RejectDuplicate with WorkflowIdReusePolicy TerminateExisting. | |
| If false (the default), an invalid argument error is returned.`, |
service/frontend/errors.go
Outdated
| errIncompatibleIDReusePolicyTerminateIfRunning = serviceerror.NewInvalidArgument("Invalid WorkflowIDReusePolicy: WORKFLOW_ID_REUSE_POLICY_TERMINATE_IF_RUNNING cannot be used together with a WorkflowIDConflictPolicy.") | ||
| errIncompatibleIDReusePolicyRejectDuplicate = serviceerror.NewInvalidArgument("Invalid WorkflowIDReusePolicy: WORKFLOW_ID_REUSE_POLICY_REJECT_DUPLICATE cannot be used together with WorkflowIdConflictPolicy WORKFLOW_ID_CONFLICT_POLICY_TERMINATE_EXISTING.") |
There was a problem hiding this comment.
nit: Typically it's recommended not to use punctuation in single sentence error strings. I know we've violated this fairly consistently but we have both forms in the codebase and I'd rather go with no punctuation, which is more standard (especially for Go codebases).
|
I made these changes:
|
common/dynamicconfig/constants.go
Outdated
| @@ -2077,6 +2077,13 @@ the user has not specified an explicit RetryPolicy`, | |||
| retrypolicy.DefaultDefaultRetrySettings, | |||
| `DefaultWorkflowRetryPolicy represents the out-of-box retry policy for unset fields | |||
| where the user has set an explicit RetryPolicy, but not specified all the fields`, | |||
| ) | |||
| EnableReusePolicyRejectOnConflictPolicyTerminate = NewNamespaceTypedSetting( | |||
There was a problem hiding this comment.
Rename to AllowReusePolicyRejectWithConflictPolicyTerminate.
There was a problem hiding this comment.
Well, by using a single flag, this is becoming awkward in terms of wording it.
We want to deny the use of the RejectDuplicate together with TerminateExisting, but honor the RejectDuplicateIfFailed policy.
"Allow" doesn't quite work here because while we will "allow" using RejectDuplicateIfFailed with TerminateExisting - we won't "allow"RejectDuplicate.
Open to suggestions.
There was a problem hiding this comment.
Maybe "Follow..."
There was a problem hiding this comment.
My comment was just to rename the var to be the same as your dynamic config name.
There was a problem hiding this comment.
Ah, that wasn't clear!
service/frontend/workflow_handler.go
Outdated
| } | ||
| if conflictPolicy == enumspb.WORKFLOW_ID_CONFLICT_POLICY_TERMINATE_EXISTING && | ||
| reusePolicy == enumspb.WORKFLOW_ID_REUSE_POLICY_REJECT_DUPLICATE && | ||
| wh.enableReusePolicyRejectOnConflictPolicyTerminate(namespaceName.String()) { |
There was a problem hiding this comment.
This should be !wh.allow..., right?
…7097) ## What changed? <!-- Describe what has changed in this PR --> Make WorkflowIdConflictPolicy TerminateExisting follow the WorkflowIdReusePolicy after an _unsuccessful_ termination. NOTE: The frontend change was made in #7099 ## Why? <!-- Tell your future self why have you made these changes --> When the termination from TerminateExisting fails, the user would expect the WorkflowIdReusePolicy to be applied as the Workflow is not running. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Well ... there's no way to test this well right now. There are no unit tests; and the functional test cannot be written without the use of [testhooks](#6938) since there is no other way to simulate the race condition here. I manually added a `sync.Once` into the code that terminates the workflow and can confirm the expected behavior. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
What changed?
Making the combination of WorkflowIdReusePolicy RejectDuplicate and WorkflowIdConflictPolicy TerminateExisting invalid.
Why?
The combination is not a desirable configuration for users. It's essentially "Terminate Workflow", ie no start would ever be performed.
How did you test it?
Added test.
Potential risks
This could break a user's behavior. Although it's unlikely. But a dynamic config was added just in case.
Documentation
Is hotfix candidate?