-
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
[POC] Show a way to implement cancellable updates #297
base: main
Are you sure you want to change the base?
Conversation
@@ -4,6 +4,9 @@ go 1.16 | |||
|
|||
replace github.com/cactus/go-statsd-client => github.com/cactus/go-statsd-client v3.2.1+incompatible | |||
|
|||
replace ( | |||
go.temporal.io/sdk v1.23.1 => ../sdk-go |
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.
Update interceptors don't work on the current release of the Go SDK
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.
Hrmm, not sure this is a pattern we should encourage, especially hiding this logic in an interceptor. I think it may be clearer if we should how to cancel an update context from another update explicitly in workflow code. If they want to make that generic to apply to all updates hidden away in an interceptor, then they can.
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 reason I moved it into the interceptor is to show you don't need to explicitly add support in your 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.
Right, but I don't think we should encourage interceptors like this nor provide a general purpose utility. I think we should show a simple example how you can cancel a single update and if someone wants to go the extra step of hiding this from workflow authors, they can.
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.
it just makes the code a less elegant to do it in the workflow, but for the samples repo your right we shouldn't encourage usage of interceptors like this
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.
Definitely less elegant and less reusable. Acknowledged. Kinda on purpose, which is admittedly a bit strange, but we don't want people to blindly copy the approach. What I've found is that many times people want to customize these patterns so seeing them in-workflow is easy to understand.
And really, if we wanted to write this properly for reuse, I might recommend a utility instead of an interceptor.
func (w *workflowInboundInterceptor) ExecuteWorkflow(ctx workflow.Context, in *interceptor.ExecuteWorkflowInput) (interface{}, error) { | ||
err := workflow.SetUpdateHandlerWithOptions(ctx, UpdateCancelHandle, func(ctx workflow.Context, updateID string) error { | ||
// Cancel the update | ||
w.ctxMap[updateID]() |
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.
Probably want to delete from ctxMap here
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 didn't delete because I wanted duplicate cancels to behave the same as the server, but I didn't implement that behaviour yet
Show how one could implement cancellable updates purely in user side code.