feat: add cancel-in-progress sample #225

StarpTech · 2022-10-23T21:08:31Z

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.

Any docs updates needed?

I'm not familiar with the template boundaries @@@SNIPSTART in the samples. Please tell me the instructions.

CLAassistant · 2022-10-23T21:08:35Z

All committers have signed the CLA.

cretz · 2022-10-24T12:42:53Z

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:

This could easily start dozens of child workflows and you'd never know if one errored. As a general purpose example, you may want to ensure only one child runs at a time.
You can completely miss child workflow failure if signal comes in at same time which is not safe for a general purpose example.
You're checking the parent context to see whether the child is cancelled.
For a general sample, it may not make sense to drain the signal channel. If I send 2 signals as a caller, it may be strange that one was ignored.

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 "Child execution cancelled." log appears.

StarpTech · 2022-10-24T14:07:45Z

Hi, @cretz it would help a lot if you could point to the relevant places for your argumentation.

This could easily start dozens of child workflows and you'd never know if one errored. As a general purpose example, you may want to ensure only one child runs at a time.

How so? A child workflow error will cancel the loop and return the error to the parent workflow

You're checking the parent context to see whether the child is canceled.

I don't think so. The cancellable context is created here https://github.com/temporalio/samples-go/pull/225/files#diff-e7190ffa6a984a8e3be3db5ad97758314062293387073e586d442ddd9d664b81R29

For a general sample, it may not make sense to drain the signal channel. If I send 2 signals as a caller, it may be strange that one was ignored.

That's the whole idea of this sample. I'm only interested in the last signal.

Since this is essentially restart-child-on-signal, not sure it deserves an entire sample.

Where can I find this example?

StarpTech · 2022-10-24T14:13:33Z

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?

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.

cretz · 2022-10-24T15:21:22Z

👍 Will do full review...

cretz · 2022-10-24T15:22:56Z

cancel-in-progress/child_workflow.go

+
+import (
+	"go.temporal.io/sdk/workflow"
+	"time"


I forget whether it's go fmt or goimports but may need to run them both so that stdlib imports are separate from others

cretz · 2022-10-24T15:23:21Z

cancel-in-progress/child_workflow.go

+	logger := workflow.GetLogger(ctx)
+
+	// Simulate some long running processing.
+	_ = workflow.Sleep(ctx, time.Second*3)


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)

cretz · 2022-10-24T15:23:56Z

cancel-in-progress/parent_workflow.go

+	"go.temporal.io/sdk/workflow"
+)
+
+const ParentWorkflowSignalName = "parent-workflow-signal"


IMO the signal name should reference what it does rather than who it's to, e.g. const RestartChildSignalName = "restart-child-signal"

cretz · 2022-10-24T15:24:33Z

cancel-in-progress/parent_workflow.go

+
+	var message string
+
+	reBuildSignalChan := workflow.GetSignalChannel(ctx, ParentWorkflowSignalName)


What do you mean here by "build"?

cretz · 2022-10-24T15:25:12Z

cancel-in-progress/parent_workflow.go

+	for !isProcessingDone {
+		ctx, cancelHandler := workflow.WithCancel(ctx)
+
+		// it is important to re-execute the child workflow after every signal


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)

cretz · 2022-10-24T15:37:07Z

cancel-in-progress/starter/main.go

+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,


The default can be left unset

cretz · 2022-10-24T15:37:19Z

cancel-in-progress/starter/main.go

+	defer c.Close()
+
+	// This Workflow ID must be fixed because we don't want to start a new workflow every time.
+	projectID := "my-project"


What is a "project"?

cretz · 2022-10-24T15:44:29Z

cancel-in-progress/parent_workflow.go

+	var isProcessingDone = false
+
+	for !isProcessingDone {
+		ctx, cancelHandler := workflow.WithCancel(ctx)


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.

cretz · 2022-10-24T15:45:10Z

cancel-in-progress/parent_workflow.go

+			}
+
+			if err != nil {
+				logger.Error("Child execution failure.", "Error", err)


You do this same log statement twice

cretz · 2022-10-24T15:47:53Z

cancel-in-progress/starter/main.go

+	workflowID := "parent-workflow_" + projectID
+	workflowOptions := client.StartWorkflowOptions{
+		ID:        workflowID,
+		TaskQueue: "child-workflow",


To avoid clash w/ other samples, can you name this the name of the sample?

StarpTech · 2022-10-24T16:11:18Z

@cretz great review. I will address this soon.

StarpTech added 2 commits October 23, 2022 22:58

feat: add sample

e0b7991

feat: fix typos

6e328c9

StarpTech added 7 commits October 23, 2022 23:08

feat: fix instructions

5199c6c

feat: fix typo

6d24fdc

chore: improve

b78c73b

chore: improve

e804d37

chore: improve

1439336

chore: improve

ca0ef52

chore: improve

ec3a10d

cretz reviewed Oct 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add cancel-in-progress sample #225

feat: add cancel-in-progress sample #225

StarpTech commented Oct 23, 2022

CLAassistant commented Oct 23, 2022 •

edited

Loading

cretz commented Oct 24, 2022 •

edited

Loading

StarpTech commented Oct 24, 2022 •

edited

Loading

StarpTech commented Oct 24, 2022 •

edited

Loading

cretz commented Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

cretz Oct 24, 2022

StarpTech commented Oct 24, 2022


		var message string

		reBuildSignalChan := workflow.GetSignalChannel(ctx, ParentWorkflowSignalName)

feat: add cancel-in-progress sample #225

Are you sure you want to change the base?

feat: add cancel-in-progress sample #225

Conversation

StarpTech commented Oct 23, 2022

What was changed

Why?

Checklist

CLAassistant commented Oct 23, 2022 • edited Loading

cretz commented Oct 24, 2022 • edited Loading

StarpTech commented Oct 24, 2022 • edited Loading

StarpTech commented Oct 24, 2022 • edited Loading

cretz commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented Oct 24, 2022

CLAassistant commented Oct 23, 2022 •

edited

Loading

cretz commented Oct 24, 2022 •

edited

Loading

StarpTech commented Oct 24, 2022 •

edited

Loading

StarpTech commented Oct 24, 2022 •

edited

Loading