Allow string as exceptions in on_failure_callback
#695
Conversation
on_failure_callback
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.
LGTM, this cleans things up considerably 😄
Completely out of curiosity, are you able to reproduce the Upstream task(s) failed
exception handling? I've tried raising an error in copy_to_s3
for example, which causes load_from_s3
to enter the upstream_failed
state, but there's no log file at all for the task. From the docs (and I was even curious enough to check the source) it clearly fires the callback in this state so this check definitely should be necessary to prevent Slack messages from all downstream failures.
Admittedly, this was some code I had where I was interacting with an older version of Airflow 😅 It's possible this interaction has changed or may just not be necessary at all 🤔 It's also possible this callback is being run on the scheduler rather than a whole worker being spun up just to handle the callback. I'll try and see where this is happening. |
Okay I've poked around a bit - I thought perhaps that the logs for callbacks on the upstream task failed tasks could be found in the scheduler logs or something, but I can't find them anywhere. The Airflow source code is a bit hard to pull apart in these cases, so it's hard to know if it's even being called, honestly! I'll leave it as-is for now though, it doesn't hurt to have. |
@AetherUnbound thanks for humoring me! Definitely not a concern for the PR, but an interesting mystery 😄 |
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.
see the failure callback still logged
I indeed did! LGTM.
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.
LGTM!
Fixes
Fixes WordPress/openverse#1453 by @AetherUnbound
Description
This PR allows strings to be passed in as the exception to an
on_failure_callback
.See WordPress/openverse#1453 for a longer description of the problem.
Testing Instructions
just test
Optionally attempt to fail a provider pull media task locally. You should see the failure callback still logged (instead of an exception). Here's an example, with extra logging added at the beginning to emphasise what was handed in:
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin