-
-
Notifications
You must be signed in to change notification settings - Fork 24
Do not retry processing when there is no picture #199
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
base: main
Are you sure you want to change the base?
Conversation
bf23124 to
3c5cba4
Compare
3c5cba4 to
2a3dbd6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
=======================================
Coverage 89.06% 89.06%
=======================================
Files 13 13
Lines 494 494
=======================================
Hits 440 440
Misses 54 54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pictures/tasks.py
Outdated
| except FileNotFoundError: | ||
| # The file no longer exists (for example, because it was deleted or replaced). | ||
| return |
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.
Just silently ignoring missing files, will make it challenging to debug potential errors. I believe all message queues we use have a native behavior to fail without retrying on explicit exceptions. I'd recommend exploring that route.
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.
@codingjoe hm, this is a good suggestion!
I am struggling with figuring out the good way of doing so in django-rq (the interface is quite different from what I used to see in the other queues).
I added an example commit of how I see it with celery and dramatiq:
7c73c82
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 don't have much experience with RQ either, let's ask @krtko1
Do you have any idea how to solve this in RQ?
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.
@amureki my search only brought up this https://github.com/rq/rq/pull/1480/files
acc5822 to
7c73c82
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.
Pull Request Overview
This PR prevents task queue retries when a FileNotFoundError occurs during picture processing, addressing scenarios where the source file has been deleted and retrying would be futile.
Key Changes:
- Configures Dramatiq tasks to not retry on
FileNotFoundErrorusing thethrowsparameter - Configures Celery tasks to not retry on
FileNotFoundErrorusing thedont_autoretry_forparameter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comes as an outcome of discussion #182