added a new condition before launching the self._scan_stale_dags() fu… #39159

tchakib · 2024-04-22T08:55:50Z

…nction

this condition will allow us to have the choice of purging expired dags or keeping them

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

…nction this condition will allow us to have the choice of purging expired dags or keeping them

boring-cyborg · 2024-04-22T08:55:54Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

potiuk · 2024-04-22T09:13:52Z

In order for that change would be considered for merging - it needs to have more explanation, documentation, configuration parameter added and described in the templates and unit tests.

add doc for disable deletion of stale dags

airflow/config_templates/config.yml

Taragolis

Also could you add test cases for that one into the https://github.com/apache/airflow/blob/main/tests/dag_processing/test_processor.py

airflow/dag_processing/manager.py

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

tchakib · 2024-04-22T14:24:09Z

Pourriez-vous également ajouter des cas de test pour celui-ci dans le https://github.com/apache/airflow/blob/main/tests/dag_processing/test_processor.py

Ok and thanks for the two suggestions :)

amoghrajesh

Good work on your first PR. Have some comments, and also unit tests are needed

airflow/config_templates/config.yml

amoghrajesh · 2024-04-23T04:42:06Z

airflow/config_templates/config.yml

@@ -500,6 +500,13 @@ core:
      type: integer
      example: ~
      default: "4096"
+    disalble_scan_stale_dags:
+      description: |
+        Stale dags are deleted by default, [core] disalble_scan_stale_dags is True if you want to keep them.


Correct this in context of above comment

amoghrajesh · 2024-04-23T04:42:17Z

airflow/config_templates/unit_tests.cfg

@@ -59,6 +59,8 @@ unit_test_mode = True
 killed_task_cleanup_time = 5
 # We only allow our own classes to be deserialized in tests
 allowed_deserialization_classes = airflow.* tests.*
+# expired dags are deleted by default, you can put true in this variable if you want to keep them.
+disalble_scan_stale_dags = False


Same comment as above

amoghrajesh · 2024-04-23T04:43:01Z

docs/apache-airflow/core-concepts/dags.rst

+
+In a versioned DAG context, there may be a need to run two versions of DAGs in parallel in two versions of workers.
+It can also be useful to keep the allowed DAGs if they are still in progress in the worker (n -1).
+To keep the allowed DAGs, you can change the value of the variable ``AIRFLOW__CORE__DISABLE_SCAN_STALE_DAGS`` to ``True``. By default, it is set to ``False``.


Mention that we can also set in config.yml

Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>

uranusjr · 2024-04-23T07:11:57Z

There are a couple of exceptions, but I think the configs are generally positive, i.e. this should use scan_stale_dags (or enable_...) and default to True.

tchakib · 2024-04-23T09:10:54Z

There are a couple of exceptions, but I think the configs are generally positive, i.e. this should use scan_stale_dags (or enable_...) and default to True.

Indeed, if it suits everyone we can go there :)

tchakib · 2024-04-24T20:39:25Z

it also needs actual unit tests to check if this works.

Ok, it's in progress 👍

airflow/config_templates/unit_tests.cfg

airflow/dag_processing/manager.py

docs/apache-airflow/core-concepts/dags.rst

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

rename scan_stale_dags

add unitest for enable_purging_stale_dags condition

airflow/config_templates/config.yml

uranusjr · 2024-04-26T06:18:33Z

docs/apache-airflow/core-concepts/dags.rst

@@ -924,3 +924,10 @@ if it fails for ``N`` number of times consecutively.
 we can also provide and override these configuration from DAG argument:

 - ``max_consecutive_failed_dag_runs``: Overrides :ref:`config:core__max_consecutive_failed_dag_runs_per_dag`.
+
+Disable deletion of stale DAGs


May want to clarify this. DAGs are always “deleted” (from the DAG file) to become stale, so we should try to explain here about the concept of DAGs being stale, and what keeping stale DAGs actually means.

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

fix doc and variables

typo

added a new condition before launching the self._scan_stale_dags() fu…

598667a

…nction this condition will allow us to have the choice of purging expired dags or keeping them

tchakib requested review from jedcunningham and ephraimbuddy as code owners April 22, 2024 08:55

boring-cyborg bot added the area:Scheduler Scheduler or dag parsing Issues label Apr 22, 2024

Merge branch 'main' into main

d361f9a

tchakib added 3 commits April 22, 2024 11:34

Update unit_tests.cfg

08ad4d9

Update config.yml

1b6d59b

Update dags.rst

32ef994

add doc for disable deletion of stale dags

tchakib requested a review from potiuk as a code owner April 22, 2024 13:29

Update dags.rst

b8df88e

Taragolis reviewed Apr 22, 2024

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

Taragolis reviewed Apr 22, 2024

View reviewed changes

airflow/dag_processing/manager.py Outdated Show resolved Hide resolved

tchakib and others added 2 commits April 22, 2024 16:21

Update airflow/config_templates/config.yml

deee1de

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Update airflow/dag_processing/manager.py

0a0af37

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Taragolis added this to the Airflow 2.10.0 milestone Apr 22, 2024

amoghrajesh reviewed Apr 23, 2024

View reviewed changes

tchakib and others added 5 commits April 23, 2024 08:24

Update airflow/config_templates/config.yml

c4d49a3

Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>

Update config.yml

8bb60da

Update unit_tests.cfg

511252b

Update manager.py

ff816a0

Update dags.rst

ea5113f

Update config.yml

f6fa572

tchakib added 3 commits April 23, 2024 11:18

Update config.yml

24327ab

Update unit_tests.cfg

e0b338b

Update manager.py

78d6a58

tchakib added 2 commits April 23, 2024 17:49

Merge branch 'main' into main

db2b496

Merge branch 'main' into main

e5c86ed

tchakib added 2 commits April 24, 2024 22:41

Merge branch 'main' into main

2ce0c3a

Update config.yml

493fba5

uranusjr reviewed Apr 25, 2024

View reviewed changes

airflow/config_templates/unit_tests.cfg Outdated Show resolved Hide resolved

uranusjr reviewed Apr 25, 2024

View reviewed changes

airflow/dag_processing/manager.py Outdated Show resolved Hide resolved

uranusjr reviewed Apr 25, 2024

View reviewed changes

docs/apache-airflow/core-concepts/dags.rst Outdated Show resolved Hide resolved

tchakib and others added 8 commits April 25, 2024 09:22

Update docs/apache-airflow/core-concepts/dags.rst

2b76552

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

Update manager.py

8a1ab56

Update unit_tests.cfg

7ae8830

rename scan_stale_dags

cf3cacd

Merge pull request #1 from tchakib/scan_stale_dags

31dd4a5

rename scan_stale_dags

add unitest for enable_purging_stale_dags condition

61d09d2

Merge pull request #2 from tchakib/scan_stale_dags

6573bf1

add unitest for enable_purging_stale_dags condition

Merge branch 'main' into main

4d79865

uranusjr reviewed Apr 26, 2024

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

uranusjr reviewed Apr 26, 2024

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

uranusjr reviewed Apr 26, 2024

View reviewed changes

tchakib and others added 5 commits April 26, 2024 08:58

Update airflow/config_templates/config.yml

fcf86c9

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

fix doc and variables

b6e5640

Merge pull request #3 from tchakib/scan_stale_dags

c70da66

fix doc and variables

Merge branch 'apache:main' into main

ce411fb

Merge branch 'main' into main

3e4fdcc

tchakib requested review from potiuk, uranusjr, Taragolis and amoghrajesh April 26, 2024 13:23

tchakib added 2 commits April 29, 2024 09:25

Merge branch 'main' into main

122577a

Update dags.rst

60f9b2f

typo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a new condition before launching the self._scan_stale_dags() fu… #39159

added a new condition before launching the self._scan_stale_dags() fu… #39159

tchakib commented Apr 22, 2024

boring-cyborg bot commented Apr 22, 2024

potiuk commented Apr 22, 2024

Taragolis left a comment

tchakib commented Apr 22, 2024 •

edited

amoghrajesh left a comment

amoghrajesh Apr 23, 2024

tchakib Apr 23, 2024

amoghrajesh Apr 23, 2024

tchakib Apr 23, 2024

amoghrajesh Apr 23, 2024

tchakib Apr 23, 2024

uranusjr commented Apr 23, 2024

tchakib commented Apr 23, 2024 •

edited

tchakib commented Apr 24, 2024

uranusjr Apr 26, 2024

added a new condition before launching the self._scan_stale_dags() fu… #39159

Are you sure you want to change the base?

added a new condition before launching the self._scan_stale_dags() fu… #39159

Conversation

tchakib commented Apr 22, 2024

boring-cyborg bot commented Apr 22, 2024

potiuk commented Apr 22, 2024

Taragolis left a comment

Choose a reason for hiding this comment

tchakib commented Apr 22, 2024 • edited

amoghrajesh left a comment

Choose a reason for hiding this comment

amoghrajesh Apr 23, 2024

Choose a reason for hiding this comment

tchakib Apr 23, 2024

Choose a reason for hiding this comment

amoghrajesh Apr 23, 2024

Choose a reason for hiding this comment

tchakib Apr 23, 2024

Choose a reason for hiding this comment

amoghrajesh Apr 23, 2024

Choose a reason for hiding this comment

tchakib Apr 23, 2024

Choose a reason for hiding this comment

uranusjr commented Apr 23, 2024

tchakib commented Apr 23, 2024 • edited

tchakib commented Apr 24, 2024

uranusjr Apr 26, 2024

Choose a reason for hiding this comment

tchakib commented Apr 22, 2024 •

edited

tchakib commented Apr 23, 2024 •

edited