-
Notifications
You must be signed in to change notification settings - Fork 202
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
feat: add cancel-in-progress sample #225
base: main
Are you sure you want to change the base?
feat: add cancel-in-progress sample #225
Conversation
Thanks for the contribution! To confirm, this example just starts a child workflow when the parent workflow starts and then waits for a signal to cancel the child workflow and start a new one? If I send 3 signals you start 3 children without waiting for any of the children to complete? Some notes:
Since this is essentially restart-child-on-signal, not sure it deserves an entire sample. But if so I think it could be simplified and clarified a bit. You might also want a test or two. For instance, make a test where you replicate the condition where the |
Hi, @cretz it would help a lot if you could point to the relevant places for your argumentation.
How so? A child workflow error will cancel the loop and return the error to the parent workflow
I don't think so. The cancellable context is created here https://github.com/temporalio/samples-go/pull/225/files#diff-e7190ffa6a984a8e3be3db5ad97758314062293387073e586d442ddd9d664b81R29
That's the whole idea of this sample. I'm only interested in the last signal.
Where can I find this example? |
No, I start a workflow with a signal. When another signal is sent during the execution of the child workflow the workflow is canceled and a new one is started with the options from the last available signal. |
👍 Will do full review... |
|
||
import ( | ||
"go.temporal.io/sdk/workflow" | ||
"time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget whether it's go fmt
or goimports
but may need to run them both so that stdlib imports are separate from others
logger := workflow.GetLogger(ctx) | ||
|
||
// Simulate some long running processing. | ||
_ = workflow.Sleep(ctx, time.Second*3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you swallow this error this cannot get cancelled. It is important for our samples to show best practices of always responding to errors (if we haven't in other samples we should fix that)
"go.temporal.io/sdk/workflow" | ||
) | ||
|
||
const ParentWorkflowSignalName = "parent-workflow-signal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the signal name should reference what it does rather than who it's to, e.g. const RestartChildSignalName = "restart-child-signal"
|
||
var message string | ||
|
||
reBuildSignalChan := workflow.GetSignalChannel(ctx, ParentWorkflowSignalName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean here by "build"?
for !isProcessingDone { | ||
ctx, cancelHandler := workflow.WithCancel(ctx) | ||
|
||
// it is important to re-execute the child workflow after every signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you don't execute after every signal. If you get two signals you only execute after the latest. (seems to be by intention, just need to change this comment)
func main() { | ||
// The client is a heavyweight object that should be created only once per process. | ||
c, err := client.Dial(client.Options{ | ||
HostPort: client.DefaultHostPort, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default can be left unset
defer c.Close() | ||
|
||
// This Workflow ID must be fixed because we don't want to start a new workflow every time. | ||
projectID := "my-project" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "project"?
var isProcessingDone = false | ||
|
||
for !isProcessingDone { | ||
ctx, cancelHandler := workflow.WithCancel(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity reasons you might want rename ctx
instead of shadowing the parent ctx
. It can be unclear whether this is wrapping one from the previous loop iteration or not.
} | ||
|
||
if err != nil { | ||
logger.Error("Child execution failure.", "Error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do this same log statement twice
workflowID := "parent-workflow_" + projectID | ||
workflowOptions := client.StartWorkflowOptions{ | ||
ID: workflowID, | ||
TaskQueue: "child-workflow", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid clash w/ other samples, can you name this the name of the sample?
@cretz great review. I will address this soon. |
What was changed
This example demonstrates how to implement a workflow that ensures that only one run of the child workflow is executed at a time. Subsequent runs will cancel the running child workflow and re-run it with the latest sent options through
SignalWithStartWorkflow
.Those semantics are useful, especially when implementing a CI pipeline. New commits during the workflow execution should short-circuit the child workflow and only build the most recent commit.
Why?
At WunderGraph, we use temporal to build our CI and CD platform. The solution has been implemented in collaboration with @mfateev I promised him to make a PR 😃
Checklist
Closes
How was this tested:
Manually, through the sample with a local temporal cluster.
I'm not familiar with the template boundaries
@@@SNIPSTART
in the samples. Please tell me the instructions.