subhramit commented on code in PR #70795:
URL: https://github.com/apache/airflow/pull/70795#discussion_r4085292687


##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -6620,7 +6620,7 @@ def test_do_schedule_max_active_runs_dag_timed_out(self, 
dag_maker, session):
         run1 = session.merge(run1)
         session.refresh(run1)
         assert run1.state == State.FAILED
-        assert run1_ti.state == State.SKIPPED
+        assert run1_ti.state == State.FAILED

Review Comment:
   This test has no mapped task. Only a single `BashOperator`.
   Could we add a regression test for the issue (a mapped task with some TIs 
running and some pending at timeout), and one for a teardown task?



##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -2948,7 +2948,7 @@ def _schedule_dag_run(
                 default=None,
             )
             for task_instance in unfinished_task_instances:
-                task_instance.state = TaskInstanceState.SKIPPED
+                task_instance.state = TaskInstanceState.FAILED

Review Comment:
   Correct me if I'm wrong, but won't this affect every DAG run timeout and not 
only mapped tasks?
   That would cause every unfinished TI of every timed-out run become FAILED, 
including TIs that never started.



##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -2948,7 +2948,7 @@ def _schedule_dag_run(
                 default=None,
             )
             for task_instance in unfinished_task_instances:
-                task_instance.state = TaskInstanceState.SKIPPED
+                task_instance.state = TaskInstanceState.FAILED

Review Comment:
   This would also change teardown tasks as an unfinished teardown now would 
show FAILED, although it never ran.
   "Mark run failed" does not change teardowns, sets running TIs to FAILED, and 
sets pending TIs to SKIPPED:
   
https://github.com/apache/airflow/blob/675d3e31675ee08e3f67fc99e2f679f7be2b2470/airflow-core/src/airflow/api/common/mark_tasks.py#L300-L318
   
   Could we maybe use the same rule here? That would also fix the original 
issue, because the running mapped TIs become FAILED.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to