-
Notifications
You must be signed in to change notification settings - Fork 0
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
Actually retry every database error, including "pq: ..." #59
base: main
Are you sure you want to change the base?
Conversation
retry/retry.go
Outdated
@@ -193,7 +194,7 @@ func Retryable(err error) bool { | |||
|
|||
var mye *mysql.MySQLError | |||
var pqe *pq.Error | |||
if errors.As(err, &mye) || errors.As(err, &pqe) { | |||
if errors.As(err, &mye) || errors.As(err, &pqe) || strings.HasPrefix(err.Error(), "pq: ") { |
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.
Could you please add a bit more information here? Does this case necessary belong to this if
?
Personally, I am a bit puzzled by this change.
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.
Personally, I am a bit puzzled by this change.
According to Icinga/icingadb#620 (comment), lib/pq
may return errors as non-helpful types. To me, that sounds more like an issue in lib/pq
where this change is a workaround at best. This definitely isn't how best-practice error handling in Go should look like, so this should say why this is necessary. Is there a bug report in lib/pq
on this error reporting that could be references for example?
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.
No, I didn't found any: https://github.com/lib/pq/issues?q=is%3Aissue+is%3Aopen+errorf
Also, it's not the worst workaround IMAO, as /^pq: / catches them all at once.
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.
This definitely isn't how best-practice error handling in Go should look like,
Not so loud, please! 🙈 https://github.com/Icinga/icingadb/blob/v1.2.0/pkg/icingaredis/utils.go#L96
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.
That's just returning an error, not error handling. Should we have the need to handle that error specifically, that should be changed to its own type rather than checking for that string, that's all.
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.
No, I didn't found any: https://github.com/lib/pq/issues?q=is%3Aissue+is%3Aopen+errorf
Now there's one: lib/pq#1169
6b1d918
to
6811eae
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.
While still being unhappy about how the error needs to be checked, the change looks good to me now.
Please rebase! |
They aren't of type *pq.Error, but they're clearly database errors.
6811eae
to
00aae3d
Compare
They aren't of type *pq.Error, but they're clearly database errors.
refs Icinga/icingadb#620
("fixes" will be a dependency update.)