You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "tchakib (via GitHub)" <gi...@apache.org> on 2024/04/22 08:55:52 UTC

[PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

tchakib opened a new pull request, #39159:
URL: https://github.com/apache/airflow/pull/39159

   …nction
   
   this condition will allow us to have the choice of purging expired dags or keeping them
   
   <!--
    Licensed to the Apache Software Foundation (ASF) under one
    or more contributor license agreements.  See the NOTICE file
    distributed with this work for additional information
    regarding copyright ownership.  The ASF licenses this file
    to you under the Apache License, Version 2.0 (the
    "License"); you may not use this file except in compliance
    with the License.  You may obtain a copy of the License at
   
      http://www.apache.org/licenses/LICENSE-2.0
   
    Unless required by applicable law or agreed to in writing,
    software distributed under the License is distributed on an
    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    KIND, either express or implied.  See the License for the
    specific language governing permissions and limitations
    under the License.
    -->
   
   <!--
   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 an 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/
   -->
   
   
   
   <!-- Please keep an empty line above the dashes. -->
   ---
   **^ Add meaningful description above**
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
   In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
   


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1574840593


##########
airflow/dag_processing/manager.py:
##########
@@ -599,7 +599,10 @@ def _run_parsing_loop(self):
 
             if self.standalone_dag_processor:
                 self._fetch_callbacks(max_callbacks_per_loop)
-            self._scan_stale_dags()
+
+            # this variable gives us flexibility to purge stale dags or not.
+            if not conf.get("core", "disable_scan_stale_dags", fallback=None):

Review Comment:
   ```suggestion
               if not conf.getboolean("core", "disable_scan_stale_dags", fallback=False):
   ```
   
   We have a parser for the potential boolean, as well as better to provide default value explicitly



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1580525996


##########
airflow/config_templates/config.yml:
##########
@@ -500,6 +500,14 @@ core:
       type: integer
       example: ~
       default: "4096"
+    enable_purging_stale_dags:
+      description: |
+        Stale dags are deleted by default, [core] enable_purging_stale_dags is False
+        if you want to keep them.

Review Comment:
   ```suggestion
           Stale dags are deleted by default, set this to False
           if you want to keep them.
   ```



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1575908534


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

Review Comment:
   fixed



##########
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

Review Comment:
   fixed



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1580580045


##########
airflow/config_templates/config.yml:
##########
@@ -500,6 +500,14 @@ core:
       type: integer
       example: ~
       default: "4096"
+    enable_purging_stale_dags:

Review Comment:
   Ok, i prefer  purge_stale_dags



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1575909062


##########
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
+------------------------------
+
+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``.

Review Comment:
   fixed



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2259398995

   This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1575631616


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

Review Comment:
   Correct this in context of above comment



##########
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
+------------------------------
+
+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``.

Review Comment:
   Mention that we can also set in config.yml



##########
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

Review Comment:
   Same comment as above



##########
airflow/config_templates/config.yml:
##########
@@ -500,6 +500,13 @@ core:
       type: integer
       example: ~
       default: "4096"
+    disalble_scan_stale_dags:

Review Comment:
   ```suggestion
       disable_scanning_stale_dags
   ```



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1578810901


##########
airflow/dag_processing/manager.py:
##########
@@ -599,7 +599,10 @@ def _run_parsing_loop(self):
 
             if self.standalone_dag_processor:
                 self._fetch_callbacks(max_callbacks_per_loop)
-            self._scan_stale_dags()
+
+            # this variable gives us flexibility to purge stale dags or not.

Review Comment:
   This is obvious from actual logic; the comment is quite redundant.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1579059967


##########
airflow/dag_processing/manager.py:
##########
@@ -599,7 +599,10 @@ def _run_parsing_loop(self):
 
             if self.standalone_dag_processor:
                 self._fetch_callbacks(max_callbacks_per_loop)
-            self._scan_stale_dags()
+
+            # this variable gives us flexibility to purge stale dags or not.

Review Comment:
   Replace deletion with _delete_ I think it makes sense. Or just *delete (or purge) stale DAGs* (since obviously you need to find them to delete them).



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #39159: added a new condition before launching the self._scan_stale_dags() fu…
URL: https://github.com/apache/airflow/pull/39159


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1636017401


##########
docs/apache-airflow/core-concepts/dags.rst:
##########
@@ -924,3 +924,21 @@ 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
+------------------------------
+
+A small clarification about obsolete DAGs: For the DAGs to become obsolete,
+they must be deleted from the DAG file. And it is at this point that the purge_stale_dags function will carry out the purge of these DAGs in the database.
+
+A use case where we want to keep obsolete DAGs:
+
+In a versioned DAG context, it may be necessary to run two versions of DAG in parallel in two versions of workers.
+
+It can also be useful to keep obsolete DAGs if they are still being used on the version (n-1) of the worker.
+
+However, as the scheduler will not have the DAGs of the version (n-1), it will automatically launch the purge of non-existing DAGs in the DAG file.
+
+To keep obsolete DAGs, we can set the value of the ``AIRFLOW__CORE__PURGE_STALE_DAGS`` variable to ``False``.
+
+By default, it is set to ``True`` in ``config.yml``.

Review Comment:
   ```suggestion
   To keep obsolete DAGs, we can set the value of the ``AIRFLOW__CORE__PURGE_STALE_DAGS`` variable to ``False``. By default, it is set to ``True``.
   ```
   
   I don’t think users generally care about where the default is set.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2069642961

   > modèles
   
   
   
   > 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 :)


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1574837102


##########
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.
+      version_added: 2.9.0

Review Comment:
   We add new features into the minor versions



##########
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.
+      version_added: 2.9.0

Review Comment:
   ```suggestion
         version_added: 2.10.0
   ```



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1580525790


##########
airflow/config_templates/config.yml:
##########
@@ -500,6 +500,14 @@ core:
       type: integer
       example: ~
       default: "4096"
+    enable_purging_stale_dags:

Review Comment:
   ```suggestion
       auto_purge_stale_dags:
   ```
   
   Or maybe just `purge_stale_dags` would be enough.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1579012744


##########
airflow/dag_processing/manager.py:
##########
@@ -599,7 +599,10 @@ def _run_parsing_loop(self):
 
             if self.standalone_dag_processor:
                 self._fetch_callbacks(max_callbacks_per_loop)
-            self._scan_stale_dags()
+
+            # this variable gives us flexibility to purge stale dags or not.

Review Comment:
   Indeed, I left this comment because it seems to me that the function name is not explicit enough in the sense that it does not only scan but also deletes the dags.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2068891548

   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.


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2071589726

   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.


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2075801959

   
   
   
   > it also needs actual unit tests to check if this works.
   
   Ok, it's in progress 👍 


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1580528227


##########
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

Review Comment:
   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.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2294005048

   Hello @ephraimbuddy,
   
   Can we know the reason for postponing this future version to 2.11 instead of 2.10?”
   I’m asking because we’re eagerly awaiting this future version in order to stop forking the official Apache Airflow version.


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2071804966

   > 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.
   
   in fact, if it suits everyone we can go there :)


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2068855641

   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]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
   - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
   - 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
   


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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "tchakib (via GitHub)" <gi...@apache.org>.
tchakib commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1579051279


##########
airflow/dag_processing/manager.py:
##########
@@ -599,7 +599,10 @@ def _run_parsing_loop(self):
 
             if self.standalone_dag_processor:
                 self._fetch_callbacks(max_callbacks_per_loop)
-            self._scan_stale_dags()
+
+            # this variable gives us flexibility to purge stale dags or not.

Review Comment:
   But can we rename it like this? 
   For exemple: _scan_stale_dags > _scan_and_deletion_stale_dags



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1578810599


##########
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 False in this variable if you want to keep them.
+enable_scanning_stale_dags = True

Review Comment:
   This change is not needed.



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39159:
URL: https://github.com/apache/airflow/pull/39159#discussion_r1578810984


##########
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

Review Comment:
   ```suggestion
   Disable deletion of stale DAGs
   ```



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


Re: [PR] added a new condition before launching the self._scan_stale_dags() fu… [airflow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39159:
URL: https://github.com/apache/airflow/pull/39159#issuecomment-2463470164

   This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.


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