Skip to content

Conversation

@jbret89
Copy link

@jbret89 jbret89 commented Aug 14, 2024

This PR aims to refactor some points showcased following:

  • Unify both contexts in a single one.
  • Add ORM abstraction to store logs.
  • Avoid compensating errored step.
  • Add compensation error in the log and its duration.

@itimofeev
Copy link
Owner

Hello!
Thank you for your contribution!
I'll leave few questions in MR before merging, could you please clarify some moments?

You can read more details about this pattern here https://microservices.io/patterns/data/saga.html#example-choreography-based-saga

# Installing
```go get github.com/itimofeev/go-saga```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should keep previous import in order to merge this MR. It'll be strange, if repository will be itimofeev, but module name will contain another name.

Name string
Type string
Time time.Time
ExecutionID string `gorm:"index;type:varchar(255)"`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some doubts for keeping gorm tags in general Log struct, because not all users use gorm for storing logs.
What do you think if move them to gorm package and convert general Log struct to gorm one somewhere in gorm related code?

logStore Store
}

func NewCoordinator(ctx context.Context, saga *Saga, logStore Store, executionID ...string) *ExecutionCoordinator {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the idea to have ctx in methods that work with store I'm not sure that removing one of context is the good idea. First context was used to process direct flow when everything happens as expected. But what if user cancels the request, in that case we need to rollback some steps but we can't use the same already canceled context, because we will not be able to commit any rollback transactions.
Could you please clarify how your solution will work in that case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants