-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Bugfix: dag_bag.get_dag should not raise exception #18554
Conversation
Also, error for missing DAG now returns a REST API not found error:
I think this PR #18523 caused it |
The API has been returning 404 for quite some time (and IMO it's the correct behaviour). #18523 only refactored the implementation to raise 404 in a different way. |
I meant on the webserver. Sorry I didn't make that clear. If you change the URL for the dag on tree/graph view such that the dag Id is not in SerializedDagModel. You will get the above error which I think should be for REST API |
96ed40d
to
7fc3341
Compare
@@ -150,10 +150,6 @@ class DuplicateTaskIdFound(AirflowException): | |||
"""Raise when a Task with duplicate task_id is defined in the same DAG""" | |||
|
|||
|
|||
class SerializedDagNotFound(DagNotFound): |
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.
Another option would be to use try_except
in all the places we called get_dag
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 explain when this happens -- when is there a row in the DAG table not not in the SerilaizedDag table?
When a DAG is deleted, trying to view the deleted DAG in the UI raises an exception. This is expected but because it breaks the UI instead of showing a simple error message that the DAG is missing, it looks like an error. On check, I discovered that there are many other places |
66ff930
to
01e3046
Compare
get_dag raising exception is breaking many parts of the codebase. The usage in code suggests that it should return None if a dag is not found. There are about 30 usages expecting it to return None if a dag is not found. A missing dag errors out in the UI instead of returning a message that DAG is missing. This PR adds a try/except and returns None when a dag is not found fixup! Bugfix: dag_bag.get_dag should not raise exception Remove SerializedDagNotFound exception fixup! Remove SerializedDagNotFound exception Update airflow/jobs/scheduler_job.py Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> fixup! Update airflow/jobs/scheduler_job.py
199aca4
to
c3f0fa5
Compare
Cherry-pick from community: apache/airflow#18554 and apache/airflow#19113 and apache/airflow#18523 Change-Id: If366bd0ec34d96f736b84c45b18305785dc6e671 GitOrigin-RevId: 3cb294ecbee35e49817880ba95db4467471e5029
get_dag raising exception is breaking many parts of the codebase.
The usage in code suggests that it should return None if a dag is not
found. There are about 30 usages expecting it to return None if a dag
is not found. A missing dag errors out in the UI instead of returning
a message that DAG is missing.
This PR returns None when a dag is not found in SerializedDagModel instead of raising exception
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.