-
Notifications
You must be signed in to change notification settings - Fork 8
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
The birth of a new river #1135
The birth of a new river #1135
Conversation
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 don't think this approach quite works.. job arguments need to be serializable.. and the worker must hold the openfga client and db connection..
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.
sorry.. misclicked.. wanted to request changes.. see comments in my previous review
- Ensure River uses the correct test DB. - Ensure River receives a database that has already been migrated.
- Fix jujuclient suites to properly clean up. - Add wait in method where we insert river job
Distributed transaction fixes
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.
lgtm bar some petty comments, nice catch @kian99 on closing pool
internal/jimm/river.go
Outdated
} | ||
|
||
// NewRiverArgs is a struct that represents | ||
type NewRiverArgs struct { |
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.
Agreed with either. The function that creates a RiverConfig or RiverParams should have New
in the name.
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.
Just trying to note down the discussion we had yesterday,
- Regarding a timeout on the context (or a full timer), I'm not sure whether we decided to add this.
- We need to check that on EventKindJobFailed we check the job failed all retries. We should probably also check whether we don't want to listen for the other job kinds?
- We need to merge this into a feature branch.
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.
Happy for this to go into the feature branch at this point.
add706a
into
canonical:feature-distributed-transactions
Description
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers