Skip to content

Conversation

@ramitkataria
Copy link
Contributor

@ramitkataria ramitkataria commented Jul 28, 2025

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.

@ramitkataria
Copy link
Contributor Author

ramitkataria commented Jul 28, 2025

Unit tests coming soon complete

@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from 14bdf25 to 5720afa Compare July 28, 2025 22:52
@uranusjr
Copy link
Member

This needs some documentation.

@ferruzzi
Copy link
Contributor

Looks good so far, pending docs and unit tests.

Copy link
Contributor

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

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.
@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from b386f55 to 3c94260 Compare August 1, 2025 17:44
@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from d081ff0 to ed9bbec Compare August 5, 2025 20:44
@ferruzzi ferruzzi force-pushed the ramitkataria/deadlines/async-notifiers branch from a048aed to 1c76f75 Compare August 6, 2025 00:15
@ramitkataria ramitkataria requested a review from ferruzzi August 13, 2025 01:41
Copy link
Contributor

@ferruzzi ferruzzi left a 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?

@ferruzzi
Copy link
Contributor

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.

@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from df8c7ca to bd5f89f Compare August 18, 2025 18:39
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor Qs

@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from 27d3767 to 65e3655 Compare August 18, 2025 21:51
@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from c3187fd to ad5caac Compare August 20, 2025 01:35
@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from ad5caac to 7c27539 Compare August 20, 2025 04:48
Copy link
Contributor

@o-nikolas o-nikolas left a 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 👍

@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from c9ad460 to 1c69759 Compare August 25, 2025 22:46
@ramitkataria ramitkataria force-pushed the ramitkataria/deadlines/async-notifiers branch from 353a245 to b88859c Compare August 26, 2025 05:19
@o-nikolas o-nikolas merged commit b83fcf9 into apache:main Aug 26, 2025
107 checks passed
@o-nikolas o-nikolas deleted the ramitkataria/deadlines/async-notifiers branch August 26, 2025 16:23
mangal-vairalkar pushed a commit to mangal-vairalkar/airflow that referenced this pull request Aug 30, 2025
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.

Includes changes needed to `SlackWebhookNotifier` as an example.

---------

Co-authored-by: ferruzzi <ferruzzi@amazon.com>
nothingmin pushed a commit to nothingmin/airflow that referenced this pull request Sep 2, 2025
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.

Includes changes needed to `SlackWebhookNotifier` as an example.

---------

Co-authored-by: ferruzzi <ferruzzi@amazon.com>
@ferruzzi ferruzzi mentioned this pull request Sep 4, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants