Skip to content
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

Fail dag test if defer without triggerer #34619

Merged

Conversation

dstandish
Copy link
Contributor

If user runs dag.test and task defers and no triggerer is running, we should fail so user does not sit there waiting forever.

@dstandish dstandish changed the title Fail dag test if defer without triggeer Fail dag test if defer without triggerer Sep 26, 2023
airflow/exceptions.py Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
@@ -3957,7 +3979,16 @@ def get_current_dag(cls) -> DAG | None:
return None


def _run_task(ti: TaskInstance, session):
def _triggerer_is_healthy():
from airflow.api.common.airflow_health import get_airflow_health
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure. we do have a plan to integrate triggerer functionality into test directly right? Because this would only be good as a temporary measure; if we want to keep this function long term I’d want some refactoring instead of importing api from models.

Copy link
Member

Choose a reason for hiding this comment

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

But in either case this really should call TriggererJobRunner.most_recent_job() directly instead of going through the additional abstraction in api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure. we do have a plan to integrate triggerer functionality into test directly right? Because this would only be good as a temporary measure; if we want to keep this function long term I’d want some refactoring instead of importing api from models

Plan might be overstating it. It's an idea. But, there's no guarantee it happens. I'm simply being pragmatic, doing the easy thing to make user's life nicer, by failing when otherwise they might just wait forever.

But in either case this really should call TriggererJobRunner.most_recent_job() directly instead of going through the additional abstraction in api.

Yeah that sounds good. I wasn't sure exactly what was best.

@dstandish dstandish force-pushed the fail-dag-test-if-defer-without-triggeer branch from 5dad289 to ef577e7 Compare September 27, 2023 06:12
@dstandish
Copy link
Contributor Author

alright, sir, PTALWYGAC

airflow/models/dag.py Outdated Show resolved Hide resolved
@dstandish dstandish merged commit e81bb48 into apache:main Sep 27, 2023
41 of 43 checks passed
@dstandish dstandish deleted the fail-dag-test-if-defer-without-triggeer branch September 27, 2023 16:25
@eladkal eladkal added this to the Airflow 2.7.2 milestone Sep 29, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
If user runs dag.test and task defers and no triggerer is running, we should fail so user does not sit there waiting forever.

---------

Co-authored-by: Tzu-ping Chung <[email protected]>
(cherry picked from commit e81bb48)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants