You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/30 00:56:45 UTC
[GitHub] [airflow] avkirilishin opened a new pull request #21214: Removes the next_dagrun_create_after reset
avkirilishin opened a new pull request #21214:
URL: https://github.com/apache/airflow/pull/21214
closes: #21083
The `next_dagrun_create_after` reset causes a problem (details in the issue).
But now we check `_should_update_dag_next_dagruns` before `calculate_dagrun_date_fields` call. So we can remove the reset.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1032303165
Can we add `test_runs_are_created_after_max_active_runs_was_reached` to #21413 from this merge request?
https://github.com/apache/airflow/blob/039ba528b870e3935780d99f02370d816feb1b61/tests/jobs/test_scheduler_job.py#L2927-L2961
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031427778
> With main plus "active_runs-1" or with this branch plus "active_runs-1"?
>
> Main plus "active_runs-1" - the manually changed DagRun state leads to a hang for me (with `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL=86400`). This branch plus "active_runs-1" - the manually changed DagRun state works fine.
It still works for me in main, not sure why. I will suggest we implement `active_runs-1` and look at fixing it when the state is manually changed.
We can also create a boolean column on DagModel and instead of nullifying next_dagrun_create_after, we nullify the column. That way, we don't need to calculate the column again. We would just set the new column when max_active_runs is reached and unset it when dagrun finishes running or are marked manually as failed.
That together with `active_runs-1` at lines 1088 and 1062 will be a better fix
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031401288
> > This is a good change, and I think we need to apply it. But only it won't work if one sets the DagRun state manually (Success or Failed).
>
> It worked for me. Can you tests too?
With main plus "active_runs-1" or with this branch plus "active_runs-1"?
Main plus "active_runs-1" - the manually changed DagRun state leads to a hang for me (with `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL=86400`).
This branch plus "active_runs-1" - the manually changed DagRun state works fine.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1032286602
I have added the PR: https://github.com/apache/airflow/pull/21413
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin closed pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin closed pull request #21214:
URL: https://github.com/apache/airflow/pull/21214
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031260283
> > I tested your suggestion. The problem has not gone away. DAG sticks after the first launch.
>
> Sorry, I have updated my comment, I gave the wrong permalink. You should change line 1088
>
> https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1088
This is a good change, and I think we need to apply it. But only it won't work if one sets the DagRun state manually (Success or 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031201184
> I tested your suggestion. The problem has not gone away. DAG sticks after the first launch.
Sorry, I have updated my comment, I gave the wrong permalink. You should change line 1088
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030786269
@avkirilishin , Can you test with `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL=86400`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1032303165
Can we add `test_runs_are_created_after_max_active_runs_was_reached` to #21413 from this merge request?
https://github.com/apache/airflow/blob/039ba528b870e3935780d99f02370d816feb1b61/tests/jobs/test_scheduler_job.py#L2927-L2961
I will remove `min_active_runs_check_interval` from it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031200233
> I have not been able to pin out why it's returning a wrong figure but If you change this line of code:
> The problem will be gone.
I tested your suggestion. The problem has not gone away. DAG sticks after the first launch.
<img width="867" alt="image" src="https://user-images.githubusercontent.com/54231417/152751930-9675ba96-7b6f-4858-af67-b00dc8987937.png">
<img width="899" alt="image" src="https://user-images.githubusercontent.com/54231417/152751897-b22dd22b-10ef-4284-bcc7-dd691475c28b.png">
<img width="1652" alt="image" src="https://user-images.githubusercontent.com/54231417/152752175-7c75e3ac-78ed-414a-b293-2d3b38e62fcc.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031569668
But we can (and we do it) calculate the current running DagRun count at any time. Why do we need a new column?
Maybe this way:
- implement `active_runs-1`
- remove nullifying `next_dagrun_create_after`
- and a little later I will redo the load testing, maybe my previous conclusions were wrong
?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031645718
Is there anything else I can do? And whether to close this merge request or not?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031201184
> I tested your suggestion. The problem has not gone away. DAG sticks after the first launch.
Sorry, I have updated my comment, I gave the wrong permalink. You should change line 1088
https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1088
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1026208376
This needs more thought/a deeper review than I can give right now.
I think by removing that then we might have undone the fix where we introduced this change in the first place
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1028276711
> I think by removing that then we might have undone the fix where we introduced this change in the first place
I checked this. Let's reconstruct the sequence of events.
- Initially next_dagrun_create_after resets only in `calculate_dagrun_date_fields` and only with other "next*" attributes (`next_dagrun`, `next_dagrun_data_interval`). It didn't control max_active_runs.
- #18897: unconditional `_update_dag_next_dagruns` call introduced, now `next_dagrun_create_after` used to control max_active_runs
- #19528: `_should_update_dag_next_dagruns` was added, so we don't call `_update_dag_next_dagruns` without check and we don't need `next_dagrun_create_after = NULL` anymore
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030805388
> Can you test with `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL=86400`
@ephraimbuddy
<img width="607" alt="image" src="https://user-images.githubusercontent.com/54231417/152676339-c51d24c1-32a6-4319-95c3-b46c1d7e643f.png">
<img width="888" alt="image" src="https://user-images.githubusercontent.com/54231417/152676727-7c98f69e-2b08-4918-a9cc-82e20ea95e72.png">
<img width="257" alt="image" src="https://user-images.githubusercontent.com/54231417/152676973-428c04cb-a712-4384-9b1e-1f4e0c99503f.png">
<img width="1481" alt="image" src="https://user-images.githubusercontent.com/54231417/152676988-83593c20-bcb4-4ddb-882e-75b586b22820.png">
<img width="981" alt="image" src="https://user-images.githubusercontent.com/54231417/152677010-7053f120-7ecb-42c6-a043-bf86ce004d78.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030925842
Hi @avkirilishin, I debugged this issue and it turned out that the problem is not that the `next_dagrun_create_after` is being nullified. The problem is that this line https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1060
is returning a wrong figure. You can verify this.
I have not been able to pin out why it's returning a wrong figure but If you change this line of code:
https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1088
to
```python
if self._should_update_dag_next_dagruns(dag, dag_model, active_runs-1):
```
The problem will be gone.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030277456
I did load testing today, and it turned out that now the scheduler consumes a lot more CPU, since it constantly selects all the necessary DAGs in the `DagModel.dags_needing_dagruns` function. Maybe I can add a dict with the time of the last check? But it won't work for `AIRFLOW__SCHEDULER__NUM_RUNS = 1`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1032303165
Can we add `test_runs_are_created_after_max_active_runs_was_reached` to #21413 from this merge request?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030277109
> but it didn't resolve the linked issue
This fix resolves the linked issue. The new test `test_runs_are_created_after_max_active_runs_was_reached` verifies this.
I have to explain how it works:
1. The value of `max_active_runs` has been reached, and the value of `next_dagrun_create_after` has been reset: https://github.com/apache/airflow/blob/ab762a5a8ae147ae33500ee3c7e7a73d25d03ad7/airflow/jobs/scheduler_job.py#L975
2. Due to filtering `next_dagrun_create_after`, `DagModel.dags_needing_dagruns` (referred to in `SchedulerJob._create_dagruns_for_dags`) cannot select Dags to create new runs. Comparison predicates with NULL values always return UNKNOWN (==FALSE in that case).
https://github.com/apache/airflow/blob/ab762a5a8ae147ae33500ee3c7e7a73d25d03ad7/airflow/models/dag.py#L2866-L2876
3. Dag sticks and runs are no longer being created
4. If `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL` is low, the scheduler processes dag's file and calls `Dag.bulk_write_to_db`, which calls `calculate_dagrun_date_fields` and calculates the new `next_dagrun_create_after` value
https://github.com/apache/airflow/blob/ab762a5a8ae147ae33500ee3c7e7a73d25d03ad7/airflow/models/dag.py#L2441-L2444
5. But if `AIRFLOW__SCHEDULER__MIN_FILE_PROCESS_INTERVAL` is high, nobody calculates `next_dagrun_create_after`
6. This merge request corrects situation by removing the `next_dagrun_create_after` nullify
---
`next_dagrun_create_after` has been reset:
<img width="920" alt="Screenshot 2022-02-04 at 21 51 39" src="https://user-images.githubusercontent.com/54231417/152586591-3d02a5ce-5873-4982-a3ba-2feb27fd0c98.png">
`min_file_process_interval` is 180. After 3 minutes (16:21:36 - 16:24:37), the scheduler processes dag's file and calculates the new `next_dagrun_create_after` value:
<img width="920" alt="Screenshot 2022-02-04 at 21 56 18" src="https://user-images.githubusercontent.com/54231417/152586987-6e62976e-04aa-4dc1-843f-6370114937c1.png">
There is a gap in launches:
<img width="1101" alt="image" src="https://user-images.githubusercontent.com/54231417/152586855-125fd1d6-6761-4cae-97d9-d8bf856c21f1.png">
Pay attention to the same `last_parsed_time` and `Queued At` in the last two screenshots.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031609792
> But we can (and we do it) calculate the current running DagRun count at any time. Why do we need a new column?
>
> Maybe this way:
>
> * implement `active_runs-1`
> * remove nullifying `next_dagrun_create_after`
> * and a little later I will redo the load testing, maybe my previous conclusions were wrong
>
> ?
I want to avoid https://github.com/apache/airflow/pull/21214/files#diff-62c8e300ee91e0d59f81e0ea5d30834f04db71ae74f2e155a10b51056b00b59bR2874. That query is run a lot, the conditional check and providing a list of IDs to look into won't be efficient in large deployments
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031731181
> Is there anything else I can do? And whether to close this merge request or not?
If you don't mind you can apply the `active_runs-1` change and leave out the addition of new column for now until this issue https://github.com/apache/airflow/issues/16078 is resolved.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031200233
> I have not been able to pin out why it's returning a wrong figure but If you change this line of code:
> The problem will be gone.
@ephraimbuddy I tested your suggestion. The problem has not gone away. DAG sticks after the first launch.
<img width="867" alt="image" src="https://user-images.githubusercontent.com/54231417/152751930-9675ba96-7b6f-4858-af67-b00dc8987937.png">
<img width="899" alt="image" src="https://user-images.githubusercontent.com/54231417/152751897-b22dd22b-10ef-4284-bcc7-dd691475c28b.png">
<img width="1652" alt="image" src="https://user-images.githubusercontent.com/54231417/152752175-7c75e3ac-78ed-414a-b293-2d3b38e62fcc.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#discussion_r798445086
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -2937,20 +2975,12 @@ def test_more_runs_are_not_created_when_max_active_runs_is_reached(self, dag_mak
self.scheduler_job.executor = MockExecutor(do_update=False)
self.scheduler_job.processor_agent = mock.MagicMock(spec=DagFileProcessorAgent)
session = settings.Session()
- assert session.query(DagRun).count() == 0
- dag_models = DagModel.dags_needing_dagruns(session).all()
- self.scheduler_job._create_dag_runs(dag_models, session)
- dr = session.query(DagRun).one()
- dr.state == DagRunState.QUEUED
- assert session.query(DagRun).count() == 1
- assert dag_maker.dag_model.next_dagrun_create_after is None
+ self.scheduler_job._do_scheduling(session)
Review comment:
Using `_do_scheduling` directly is flaky and your code here didn't check that when max_active_runs is reached that more dagruns are not created
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031733202
> > Is there anything else I can do? And whether to close this merge request or not?
>
> If you don't mind you can apply the `active_runs-1` change and leave out the addition of new column for now until this issue #16078 is resolved.
You can do it in a new PR if you want
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1030925842
Hi @avkirilishin, I debugged this issue and it turned out that the problem is not that the `next_dagrun_create_after` is being nullified. The problem is that this line https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1060
is returning a wrong figure. You can verify this.
I have not been able to pin out why it's returning a wrong figure but If you change this line of code:
https://github.com/apache/airflow/blob/1d170f899bcc87110e55192517270ec89d511ca8/airflow/jobs/scheduler_job.py#L1062
to
```python
if self._should_update_dag_next_dagruns(dag, dag_model, active_runs-1):
```
The problem will be gone.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] avkirilishin edited a comment on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
avkirilishin edited a comment on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031569668
But we can (and we do it) calculate the current running DagRun count at any time. Why do we need a new column?
Maybe this way:
- implement `active_runs-1`
- remove nullifying `next_dagrun_create_after`
- and a little later I will redo the load testing, maybe my previous conclusions were wrong
?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1031376406
> This is a good change, and I think we need to apply it. But only it won't work if one sets the DagRun state manually (Success or Failed).
It worked for me. Can you tests too?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #21214: Removes the next_dagrun_create_after reset
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #21214:
URL: https://github.com/apache/airflow/pull/21214#issuecomment-1032315390
Done
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org