Expose CancelRequested boolean in WorkflowExecutionInfo #339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yycptt
reviewed
Dec 19, 2023
| @@ -57,6 +57,7 @@ message WorkflowExecutionInfo { | |||
| int64 history_size_bytes = 15; | |||
| // If set, the most recent worker version stamp that appeared in a workflow task completion | |||
| temporal.api.common.v1.WorkerVersionStamp most_recent_worker_version_stamp = 16; | |||
| bool cancel_requested = 17; | |||
There was a problem hiding this comment.
This message is also used by List workflow APIs, will we populate this field for List APIs as well from visibility records (probably need a new search attribute for that)?
There was a problem hiding this comment.
Agreed ,this should come back from list as well (not sure a new search attribute is needed though, can just persist with the rest of the info I think)
There was a problem hiding this comment.
would i have to add this as a field in the executions_visibility table? I'm working on adding the field to the visibility record structs in the temporal repo, but honestly I'm kind of lost
There was a problem hiding this comment.
Do we want to index it or just store for consistency? I can't find good scenario for "give me all workflows that got cancel request but haven't processed it yet".
There was a problem hiding this comment.
Do we want to index it or just store for consistency?
IMO we just need to store for consistency, not indexed at this time.
ychebotarev
added a commit
that referenced
this pull request
Dec 18, 2024
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** Add new structure -WorkflowExecutionExtendedInfo That structure has few fields: * execution timeout time * run timeout time * last reset time * cancel requested (the last one from #339) <!-- Tell your future self why have you made these changes --> **Why?** Per customer request. Execution timeout timestamp may change after workflow reset, and in this case customers have no idea when it will fire. WorkflowExecutionInfo is reused in other server calls, like ListWorkflow, etc. Because of that we either add any new field to ES, or it will be nil which is bad user experience. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** No <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** N/A
temporal-cicd bot
pushed a commit
to temporalio/api-go
that referenced
this pull request
Dec 18, 2024
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** Add new structure -WorkflowExecutionExtendedInfo That structure has few fields: * execution timeout time * run timeout time * last reset time * cancel requested (the last one from temporalio/api#339) <!-- Tell your future self why have you made these changes --> **Why?** Per customer request. Execution timeout timestamp may change after workflow reset, and in this case customers have no idea when it will fire. WorkflowExecutionInfo is reused in other server calls, like ListWorkflow, etc. Because of that we either add any new field to ES, or it will be nil which is bad user experience. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** No <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** N/A
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Add CancelRequested boolean to WorkflowExecutionInfo message
Because there was previously no way to see whether a cancellation was requested, even though the server already tracks that internally. You could only see whether the cancel had finished via the WorkflowExecutionStatus, but that left the case where the cancel was requested but not completed ambiguous.
Fixes temporalio/temporal#1018
I don't think so, since it's a new field, not a removed field. If it's blank, the value will just be false.