-
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
add saga interceptor #222
base: main
Are you sure you want to change the base?
add saga interceptor #222
Conversation
saga/interceptor.go
Outdated
@@ -0,0 +1,199 @@ | |||
package saga |
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.
Still not completely convinced hiding this stuff behind an interceptor is the clearest way. Here's an alternative from a comment on the other PR at temporalio/sdk-go#936 (comment) that I didn't test (just wrote there in comment):
func testWorkflow(ctx workflow.Context, a int) error {
var txn txn
zap.L().Debug("enter workflow")
ctx = workflow.WithActivityOptions(ctx, workflow.ActivityOptions{
StartToCloseTimeout: time.Minute,
})
var id string
if err := workflow.ExecuteActivity(ctx, "createOrder", a).Get(ctx, &id); err != nil {
return err
}
zap.L().Debug("create order, id:", zap.String("id", id))
defer txn.executeRollbackActivity(ctx, "deleteOrder", id)
if err := workflow.ExecuteActivity(ctx, "stockDeduct", a).Get(ctx, nil); err != nil {
return err
}
defer txn.executeRollbackActivity(ctx, "stockInc", a)
if err := workflow.ExecuteActivity(ctx, "createPay", a).Get(ctx, nil); err != nil {
return err
}
txn.markSuccessful()
return nil
}
type txn bool
func (t *txn) markSuccessful() { *s = true }
func (t *txn) executeRollbackActivity(ctx workflow.Context, activity interface{}, args ...interface{}) {
if !*txn {
ctx, cancel := workflow.NewDisconnectedContext(ctx)
defer cancel()
if err := workflow.ExecuteActivity(ctx, activity, args...).Get(ctx); err != nil {
workflow.GetLogger(ctx).Error("failed to convert to compensate req", zap.Error(err))
}
}
}
You can obviously convert the arg wherever you want. This uses much more familiar Go constructs like defer
and is clearer to the developer what is called on rollback. There is no hiding things on interceptor and there is no pre-registration of what is run on which activity, etc.
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 understand the concerns, but interceptor itself is hidden behind workflow. Action/Compensation is registered aside with workflow like the testing code, the removal of defer
is by design.
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.
While I understand it is by design, the question is if that is the best design. Explicit defer
at the callsite is clearer to a code writer than a hidden equivalent at the registration site. Using interceptors for this, besides hiding what is happening from someone reading the workflow code and matching against history, also is a bit error prone due to making sure that interceptors are always setup the same way for all workers and that the compensations never change in successive deployments. Not to mention interceptors also add more code, more complexity, and are more limiting feature wise (e.g. can't skip a compensation if it doesn't make sense, can't use a local variable as a parameter to the compensation, etc).
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 believe it is good enough to quick start. When number of code in workflow is large, and activity executions is in multi functions, defer
is not a easy way to handle error and do compensations. Interceptor is designed to seperate non business logic apart.
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.
So interceptor is not welcome to implement saga pattern?
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 believe it is good enough to quick start
Sure, the point is that it's easy to run something on complete when your code fails
defer
is not a easy way to handle error and do compensations.
I disagree, I think it s very easy and common in Go. Many Go functions run defer after success of something but on failure of the function as a whole.
Interceptor is designed to seperate non business logic apart.
I am not sure we want to encourage that separation. Hiding some steps of a workflow can make it hard to match against history for readers. We want workflow actions to be explicit not implicit.
So interceptor is not welcome to implement saga pattern?
I am not sure it's the best way to implement the saga pattern as it hides so much from the user as opposed to the defer pattern. These samples are meant to document Temporal best practices, and I am not sure we want to encourage use of interceptors this way.
Let me see if I can get others' opinions here.
What was changed
add worker interceptor for hosting saga workflow
Why?
saga sample has many duplicate error handle,
besides, every one will have to learn Saga pattern and then to read© code from others, it sucks.
Interceptor and local state is suitable for temporal to impl Saga pattern.
Checklist
Closes
How was this tested: