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

Replace external_trigger check with DagRunType #45961

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jason810496
Copy link
Contributor

closes: #45932


^ 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.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:db-migrations PRs with DB migration area:dev-tools area:providers area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation provider:openlineage AIP-53 labels Jan 23, 2025
@Lee-W Lee-W added the legacy api Whether legacy API changes should be allowed in PR label Jan 23, 2025
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch 3 times, most recently from 45cf835 to 9eef4f1 Compare January 24, 2025 06:48
@jason810496 jason810496 changed the title [WIP] Replace external_trigger check with DagRunType Replace external_trigger check with DagRunType Jan 24, 2025
@jason810496 jason810496 marked this pull request as ready for review January 24, 2025 08:56
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.

This also needs a newsfragment noting about the removal of it please

airflow/example_dags/plugins/event_listener.py Outdated Show resolved Hide resolved
airflow/jobs/scheduler_job_runner.py Outdated Show resolved Hide resolved
@@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis
rid of the outliers on the stats side through dashboards tooling.

Note that the stat will only be emitted for scheduler-triggered DAG runs
(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0).
(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0).
(i.e. when ``run_type`` is not *MANUAL* and ``clear_number`` is equal to 0).

Though this brings up another question: How should this function deal with run_type==BACKFILL?

Copy link
Contributor Author

@jason810496 jason810496 Jan 24, 2025

Choose a reason for hiding this comment

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

How about adding an externally_triggered_type in airflow.utils.types? Instead of checking run_type == DagRunType.MANUAL, we could use run_type in externally_triggered_type. WDYT?

externally_triggered_type: frozenset[DagRunType] = frozenset([DagRunType.BACKFILL_JOB, DagRunType.MANUAL, DagRunType.ASSET_TRIGGERED])

airflow/utils/db_cleanup.py Outdated Show resolved Hide resolved
@@ -53,7 +54,7 @@ def choose_branch(self, context: Context) -> str | Iterable[str]:
# If the DAG Run is externally triggered, then return without
# skipping downstream tasks
dag_run: DagRun = context["dag_run"] # type: ignore[assignment]
if dag_run.external_trigger:
if dag_run.run_type == DagRunType.MANUAL:
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here about BACKFILL. Need to decide how to handle this

@ashb ashb added the airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes label Jan 24, 2025
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch from a65904d to 2fee34c Compare January 24, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes area:API Airflow's REST/HTTP API area:CLI area:db-migrations PRs with DB migration area:dev-tools area:providers area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation legacy api Whether legacy API changes should be allowed in PR provider:openlineage AIP-53
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace external_trigger check with DagRunType
3 participants