Add example test for ErrorHandler
#16
Conversation
example_error_handler_test.go
Outdated
| // Either function can also set the job to be immediately cancelled, which | ||
| // we take advantage of here to make sure it's not retried in the example. | ||
| // Can also be `return nil`. | ||
| return &river.ErrorHandlerResult{SetCancelled: true} |
There was a problem hiding this comment.
See note here, but sort of had to set cancelled because otherwise there's a chance the job could retry before the example finishes up.
I guess I could also set max attempts to 1. Not sure which one is better.
There was a problem hiding this comment.
I'm torn. It's useful for this example to also demonstrate SetCancelled because there's nowhere else we'd do that. However I worry a bit about people copy-pasta'ing this without realizing what they're doing, particularly in HandlePanic where there's no comment about it.
Maybe the answer is to do both? If we set MaxAttempts to 1 for the entire worker, then we could easily show off both the SetCancelled: true usage as well as the more common usage where you probably don't want to cancel any job that panics or errors.
If we go that route, I'd say it makes the most sense to put the SetCancelled in HandlePanic so that HandleError can demonstrate the most common usage.
There was a problem hiding this comment.
+1 all that. Changed to max attempts of 1, and made HandleError return nil for copy/pasta convenience.
|
|
||
| riverClient, err := river.NewClient(riverpgxv5.New(dbPool), &river.Config{ | ||
| ErrorHandler: &CustomErrorHandler{}, | ||
| Logger: slog.New(&slogutil.SlogMessageOnlyHandler{Level: 9}), // Suppress logging so example output is cleaner (9 > slog.LevelError). |
There was a problem hiding this comment.
This is a bit unconventional, but there's a fair bit of error output on error/panic from jobExecutor. Setting level to > slog.LevelError suppresses it and makes the test output much cleaner, but in case of something bad happening might also suppress other things I suppose. Not sure which is the lesser of two evils, but this is the one I went with.
There was a problem hiding this comment.
The verbose error/panic output might be something we need to take a look at. I don't think there was much intentional design there, more that it just evolved that way as we pieced together functionality.
There was a problem hiding this comment.
True. I don't think we're too far off though. If I was running this thing I'd expect to that there was reasonable error logging by default so problems aren't black holed without me having to chase down more configuration.
brandur
force-pushed
the
brandur-error-handler-context
branch
from
34d4dcc
to
e2a00fc
Compare
brandur
force-pushed
the
brandur-error-handler-example
branch
from
c0b13bb
to
46048c6
Compare
|
|
||
| riverClient, err := river.NewClient(riverpgxv5.New(dbPool), &river.Config{ | ||
| ErrorHandler: &CustomErrorHandler{}, | ||
| Logger: slog.New(&slogutil.SlogMessageOnlyHandler{Level: 9}), // Suppress logging so example output is cleaner (9 > slog.LevelError). |
There was a problem hiding this comment.
The verbose error/panic output might be something we need to take a look at. I don't think there was much intentional design there, more that it just evolved that way as we pieced together functionality.
example_error_handler_test.go
Outdated
| // Either function can also set the job to be immediately cancelled, which | ||
| // we take advantage of here to make sure it's not retried in the example. | ||
| // Can also be `return nil`. | ||
| return &river.ErrorHandlerResult{SetCancelled: true} |
There was a problem hiding this comment.
I'm torn. It's useful for this example to also demonstrate SetCancelled because there's nowhere else we'd do that. However I worry a bit about people copy-pasta'ing this without realizing what they're doing, particularly in HandlePanic where there's no comment about it.
Maybe the answer is to do both? If we set MaxAttempts to 1 for the entire worker, then we could easily show off both the SetCancelled: true usage as well as the more common usage where you probably don't want to cancel any job that panics or errors.
If we go that route, I'd say it makes the most sense to put the SetCancelled in HandlePanic so that HandleError can demonstrate the most common usage.
brandur
force-pushed
the
brandur-error-handler-example
branch
from
46048c6
to
ecc13c7
Compare
I was writing docs on how to handle errors and panics in Go and realized that we didn't have an example demonstrating the use of `ErrorHandler` that I could copy/link to. Here, add one in.
brandur
force-pushed
the
brandur-error-handler-example
branch
from
ecc13c7
to
deeb6ac
Compare
|
Thanks! |
I was writing docs on how to handle errors and panics in Go and realized
that we didn't have an example demonstrating the use of
ErrorHandlerthat I could copy/link to. Here, add one in.