-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WorkflowIdConflictPolicy #359
Conversation
1eb0943
to
8bef071
Compare
(I have avoided this PR because it is not marked ready for review, but let me know if we should review anyways) |
@cretz I was going to open the PR for review once I implemented the Server implementation, as I understand that's minimum requirement to get API changes merged. Having said that, I don't mind getting feedback now. |
b0e7d2f
to
ca9042a
Compare
ca9042a
to
64c4e8a
Compare
## What changed? Added a test for Start Workflow + Terminate-if-Running. Also refactored the existing test cases to make them more explicit, have less duplication, less clever looping. ## Why? Adding a test for Terminate-if-Running behavior for changing its behavior in an upcoming PR (related to temporalio/api#359). ## How did you test it? Is test. ## 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? - clearer distinction between the behavior for closed and running workflows - name that isn't particular to workflowIdReusePolicy, but the more general issue of an "ID duplication" ## Why? Preparing for changes from temporalio/api#359 by pulling this refactor in early, reducing the size of the change later. ## How did you test it? Existing tests + added a test for Terminate-if-Running earlier in #5483. ## 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) -->
64c4e8a
to
92da949
Compare
92da949
to
7c46b1d
Compare
7c46b1d
to
9b1a4ef
Compare
Co-authored-by: David Reiss <[email protected]>
I just pushed another set of changes to I'd appreciate another review of that new addition. |
## What changed? Added a test for Start Workflow + Terminate-if-Running. Also refactored the existing test cases to make them more explicit, have less duplication, less clever looping. ## Why? Adding a test for Terminate-if-Running behavior for changing its behavior in an upcoming PR (related to temporalio/api#359). ## How did you test it? Is test. ## 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) -->
- clearer distinction between the behavior for closed and running workflows - name that isn't particular to workflowIdReusePolicy, but the more general issue of an "ID duplication" Preparing for changes from temporalio/api#359 by pulling this refactor in early, reducing the size of the change later. Existing tests + added a test for Terminate-if-Running earlier in temporalio#5483. <!-- Assuming the worst case, what can be broken when deploying this change to production? --> <!-- 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 this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> This is the implementation for the API change in temporalio/api#359: - added support for new WorkflowIDConflictPolicy - `FAIL` - `USE_EXISTING` - `TERMINATE_EXISTING` - set new field `Started` in response accordingly - covering Start-Workflow and Signal-With-Start - migrating WorkflowIDReusePolicy Terminate-If-Running to WorkflowIDConflictPolicy Terminate-Existing - disallowing using WorkflowIDReusePolicy Terminate-If-Running with any WorkflowIDReusePolicy ## Why? <!-- Tell your future self why have you made these changes --> See temporalio/api#359 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Existing + new tests for requirements mentioned above. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> Breaking existing usage - but the tests should have uncovered any issues. ## 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? <!-- Describe what has changed in this PR --> Adjust the Action metric for Start Workflow. This metric is used by OSS users to estimate their Cloud bill. ## Why? <!-- Tell your future self why have you made these changes --> After temporalio/api#359 was merged, a successful gRPC response from Start Workflow doesn't always result in an Action. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> Low. It's only an internal metric. ## 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?
Introducing the
WorkflowIdConflictPolicy
enum to define the dedup behavior of Start Workflow for a running workflow - and solidifying theWorkflowIdReusePolicy
as the dedup behavior for closed workflows.NOTE: Terminate-If-Running will be deprecated once all API, Server and SDK changes have been merged since otherwise users wouldn't be able to use the new enum but see a deprecation notice already.
Implementations:
Why?
This is in preparation for Update-With-Start; so that the user can indicate that an already-running Workflow is not a failure. By adding this conflict resolution mechanism directly to Start Workflow itself, it allows to combine arbitrary operations, such as Update and Signal, with Start - instead of requiring hard-coded top-level APIs like Signal-With-Start.
Without the new enum, the Server wouldn't know whether the user requested a "Maybe Start" (ie continue when Start failed because of another running Workflow with the same ID) or a "Require Start" (ie stop when Start failed because of another running Workflow with the same ID).
Breaking changes