-
Notifications
You must be signed in to change notification settings - Fork 16.6k
AIP-86 - Add async support for Notifiers#53831
Conversation
This makes the BaseNotifier awaitable and implements the necessary changes required for the notifiers to work in non-blocking mode (required for DeadlineAlerts). Since notifiers use hooks which may need to use the TaskSDK API if they will fetching a Connection, I've added async counterparts to relevant TaskSDK functions as well, while avoiding as much code duplication as I could.
I've included changes needed to SlackWebhookNotifier as an example and will add support for some other common ones soon.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.
|
Unit tests |
14bdf25 to
5720afa
Compare
|
This needs some documentation. |
|
Looks good so far, pending docs and unit tests. |
o-nikolas
left a comment
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 wonder if @ashb or @amoghrajesh want to have a look at the TaskSDK pieces here.
changes required for the notifiers to work in non-blocking mode
(required for DeadlineAlerts). Since notifiers use hooks which may need
to use the TaskSDK API if they will fetching a `Connection`, I've added
async counterparts to relevant TaskSDK functions as well, while avoiding
as much code duplication as I could.
I've included changes needed to `SlackWebhookNotifier` as an example
and will add support for some other common ones soon.
b386f55 to
3c94260
Compare
d081ff0 to
ed9bbec
Compare
a048aed to
1c76f75
Compare
ferruzzi
left a comment
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 think we're good?
|
I feel like it would be inappropriate for me to merge since I wrote some of the code, so I'm going to wait on at least someone else to approve it. |
df8c7ca to
bd5f89f
Compare
ashb
left a comment
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.
Few minor Qs
airflow-core/docs/administration-and-deployment/logging-monitoring/callbacks.rst
Outdated
Show resolved
Hide resolved
27d3767 to
65e3655
Compare
c3187fd to
ad5caac
Compare
ad5caac to
7c27539
Compare
o-nikolas
left a comment
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.
There is a lot to review here, but it seems reasonable
c9ad460 to
1c69759
Compare
353a245 to
b88859c
Compare
changes required for the notifiers to work in non-blocking mode
(required for DeadlineAlerts). Since notifiers use hooks which may need
to use the TaskSDK API if they will fetching a `Connection`, I've added
async counterparts to relevant TaskSDK functions as well, while avoiding
as much code duplication as I could.
Includes changes needed to `SlackWebhookNotifier` as an example.
---------
Co-authored-by: ferruzzi
changes required for the notifiers to work in non-blocking mode
(required for DeadlineAlerts). Since notifiers use hooks which may need
to use the TaskSDK API if they will fetching a `Connection`, I've added
async counterparts to relevant TaskSDK functions as well, while avoiding
as much code duplication as I could.
Includes changes needed to `SlackWebhookNotifier` as an example.
---------
Co-authored-by: ferruzzi