Skip to content

Conversation

@alexgleith
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Project coverage is 61.88%. Comparing base (9ee6375) to head (7643227).
Report is 156 commits behind head on develop.

Files with missing lines Patch % Lines
odc/stats/proc.py 0.00% 27 Missing ⚠️
Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@supermarkion supermarkion left a 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.

Copy link
Contributor

@emmaai emmaai left a 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.

@alexgleith
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants