[AIRFLOW-78] airflow clear leaves dag_runs #1716
Conversation
Fix a bug in the scheduler where dag runs cleared via CLI would be picked up without checking max_active_dag_runs first, resulting in too many simultaneous dag runs.
|
https://travis-ci.org/normster/incubator-airflow/builds/151091603 passed.. |
|
@r39132 @normster I believe the logic here is incorrect, as it causes all scheduling to stop (eg starting task instances for running dagruns, changing dagrun state) https://issues.apache.org/jira/browse/AIRFLOW-434 |
|
@zodiac hmn... interesting. The motivation for the original patch was to have |
I'm not sure I understand, what is "this limitation"? I tested the test cases included in JIRA on the commit right before this PR, and the test case behaved as expected. I am not sure what is the best way to achieve what this PR does while avoiding the bug. I don't have a fix for it. I'm curious how you avoided running into the bug in your production environment because in my testing, once |
|
@plypaul @zodiac @r39132 @normster @mistercrunch I would like to revert this change. It has caused a serious regression as tasks are not being scheduled anymore in master. This has been encountered in multiple reports and several PRs try to address this, however none of them have come to a conclusive end. I do understand the need to fix the bug mentioned in this PR however I consider not scheduling tasks more important than adhering to max_active_runs. Before accepting a fix to this it should be accompanied by test evidence. At a minimum a unit test should be added that verifies tasks from running dag_runs are scheduled even if max_dag_runs has been reached. A solution might be as simple as moving _process_task_instances to above all the tests or reorder them so that the check for max_dag_runs occurs after the processing of the tasks. This however should be part of this PR and properly tested. |
|
@bolkedebruin I'm okay with reverting this change. |
|
👍 Lets reopen the jira this closes too On Thursday, 8 September 2016, Sid Anand [email protected] wrote:
Im Xuan Ji! |
Fix a bug in the scheduler where dag runs cleared via CLI would be picked up without checking max_active_dag_runs first, resulting in too many simultaneous dag runs. Dear Airflow Maintainers, Please accept this PR that addresses the following issues: - https://issues.apache.org/jira/browse/AIRFLOW-78 Testing Done: - Expanded the jobs.test_scheduler_verify_max_active_runs test to test if scheduler respects max_active_dag_runs Fix a bug in the scheduler where dag runs cleared via CLI would be picked up without checking max_active_dag_runs first, resulting in too many simultaneous dag runs. Closes apache#1716 from normster/clear_dagrun
4 participants
Dear Airflow Maintainers,
Please accept this PR that addresses the following issues:
Testing Done:
Fix a bug in the scheduler where dag runs cleared via CLI would be picked up without checking max_active_dag_runs first, resulting in too many simultaneous dag runs.