-
Notifications
You must be signed in to change notification settings - Fork 48
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
order: Mark missing orders as failed in the client db #360
Conversation
d8227c8
to
a2e9bc6
Compare
clientdb/order.go
Outdated
@@ -65,6 +66,13 @@ var ( | |||
orderTlvKey = []byte("order-tlv") | |||
) | |||
|
|||
// IsNotFoundErr tries to match the given error with ErrNoOrder. | |||
func IsOrderNotFoundErr(err error) bool { |
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 looks like we are not using grpc error codes. I am not sure if there is any other way to check for what type of error we are receiving from the server that string matching 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.
Yeah, we currently don't use any custom codes on the server side. String matching is okay here. Though I'm not sure if this function better fits into the auctioneer
package?
409ffdb
to
ba88b16
Compare
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.
Nice fix, LGTM 🎉
clientdb/order.go
Outdated
@@ -65,6 +66,13 @@ var ( | |||
orderTlvKey = []byte("order-tlv") | |||
) | |||
|
|||
// IsNotFoundErr tries to match the given error with ErrNoOrder. | |||
func IsOrderNotFoundErr(err error) bool { |
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.
Yeah, we currently don't use any custom codes on the server side. String matching is okay here. Though I'm not sure if this function better fits into the auctioneer
package?
server.go
Outdated
// rest of valid orders. | ||
// | ||
// NOTE: This should never happen. | ||
case clientdb.IsOrderNotFoundErr(err): |
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.
Yeah, reading this I first thought this meant "the local DB didn't find the order". So moving the function to the auctioneer package should make that more clear.
server.go
Outdated
// as failed and allow the client start up with the | ||
// rest of valid orders. | ||
// | ||
// NOTE: This should never happen. |
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.
nit: "This should never happen under normal conditions. This was added to handle a specific failure that's being investigated"?
ba88b16
to
1b578c4
Compare
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, sorry for the late review, was buried under my other notifications 👍
The client and server db can get out of sync if some orders are not recorded properly in the server. This becomes problematic because the deamon won't start until all the bad invoices have been cleaned out. Right now the clients need to fix these edge cases manually.
The idea is to simply mark those invoices as failed and let the client continue with the start up.