-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Add listeners for Dag import errors #39739
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
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
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 would like to see some unit tests for this change as it is a change in behaviour.
Also, some expert in scheduler area to review it @uranusjr at the very least
Approved the workflow run also, watch out for CI results |
@amoghrajesh Thank you for reviewing the PR. I was trying to add test cases, but was not able to find an way how to pass a broken DAG . If someone can guide me on the same, that will be helpful. |
@amoghrajesh @uranusjr I have added the unit tests for the change and below is the test result from my local machine.
|
Implementation looks OK to me at first glance. |
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.
Looks good to me. @uranusjr WDYT?
@pushpendu91 can you fix static checks? |
@eladkal I have fixed the static checks errors. Kindly request you to review the change. |
@amoghrajesh @eladkal Request you to please start the workflow, so that static checks can be validated. I ran in my local using pre-commit but I was not getting any error. |
@amoghrajesh request you to please check the workflow. As it is not giving error in my local I am relying on the actual workflow here in github. |
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.
Can you also add the needed doc entry in https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/listeners.html
@eladkal I have added the required documentation. Thank you. |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* Adding new feature to get notification incase of import errors --------- Co-authored-by: PUSHPENDU DHARA <[email protected]>
* Adding new feature to get notification incase of import errors --------- Co-authored-by: PUSHPENDU DHARA <[email protected]>
* Adding new feature to get notification incase of import errors --------- Co-authored-by: PUSHPENDU DHARA <[email protected]>
This PR expands listeners API to be notified about DAG Import error when updating the import_errors table.
At present to identify DAG import errors, user have to refresh the UI or use Importerror REST API to get the import error details. And as there is no definite timeline( by next dag_parsing_interval) when the DAG will be parsed and import errors will be raised users need to keep on refreshing the UI or hit the API.
With this PR Airflow itself will notify the listener about the addition of import error or existing import errors. And using the below events user can take action accordingly.
a. on_new_dag_import_error
b. on_existing_dag_import_error
^ 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 newsfragments.