-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add exception handling to task running #34
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #34 +/- ##
===========================================
- Coverage 62.01% 61.88% -0.13%
===========================================
Files 20 20
Lines 1935 1939 +4
===========================================
Hits 1200 1200
- Misses 735 739 +4
Continue to review full report at Codecov.
|
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.
Should be worth to have a try. Better than no try-catch.
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 have my doubt on this "catching everything then move on" approach. And I'm not sure about the context, if it's the only resort to fix the problem, then yes, if we could do more work to avoid crash, then we should.
I agree that crashes shouldn't happen, but if they do, we have a choice of how we handle them. Currently, it's basically by aborting the whole job. What should happen is that Task should fail, be logged and the next Task can be tried. This is especially important for streaming from SQS, which has built-in retries and failed task logging using the deadletter queue. |
Not sure if this is naive, but I have some tasks failing, and Statistician currently crashes, rather than marking the task as an error and continuing.
I think this is the right place to catch exceptions and gracefully return an error notification.