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.
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
Changes from 1 commit
4519cab
88cff07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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):
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.
Sure, the point is that it's easy to run something on complete when your code fails
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.
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.
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.