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 2021/09/14 13:20:36 UTC

[GitHub] [airflow] ephraimbuddy opened a new pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

ephraimbuddy opened a new pull request #18243:
URL: https://github.com/apache/airflow/pull/18243


   The file locations of DAGs in zipped folders are not correctly listed when removing deleted dags from
   serialized dag table thus the delete query for deleting deleted dags from serialized DAGs is deleting
   dags in zipped folders.
   
   This PR fixes it by listing all the file paths as stored in SDM so that the delete query will work properly
   
   Related: https://github.com/apache/airflow/issues/18114#issuecomment-918090551
   
   
   ---
   **^ 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] github-actions[bot] commented on pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#issuecomment-919505236


   The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.


-- 
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 #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#discussion_r708558352



##########
File path: airflow/models/dag.py
##########
@@ -2814,10 +2815,24 @@ def deactivate_deleted_dags(cls, alive_dag_filelocs: List[str], session=None):
         """
         log.debug("Deactivating DAGs (for which DAG files are deleted) from %s table ", cls.__tablename__)
 
+        dag_filelocs = []
+        for fileloc in alive_dag_filelocs:
+            if zipfile.is_zipfile(fileloc):
+                with zipfile.ZipFile(fileloc) as z:
+                    dag_filelocs.extend(

Review comment:
       Mistake adding it here, it's the `DagCode.remove_deleted_dags` that needed it too. Will move the logic to `_refresh_dag_dir`. Thanks




-- 
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] jedcunningham commented on a change in pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#discussion_r708623755



##########
File path: airflow/dag_processing/manager.py
##########
@@ -660,13 +661,29 @@ def _refresh_dag_dir(self):
                 self.clear_nonexistent_import_errors()
             except Exception:
                 self.log.exception("Error removing old import errors")
+            # Check if file path is a zipfile and get the full path of the python file.

Review comment:
       ```suggestion
   
               # Check if file path is a zipfile and get the full path of the python file.
   ```
   nit




-- 
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] jedcunningham commented on a change in pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#discussion_r708463072



##########
File path: airflow/models/dag.py
##########
@@ -2814,10 +2815,24 @@ def deactivate_deleted_dags(cls, alive_dag_filelocs: List[str], session=None):
         """
         log.debug("Deactivating DAGs (for which DAG files are deleted) from %s table ", cls.__tablename__)
 
+        dag_filelocs = []
+        for fileloc in alive_dag_filelocs:
+            if zipfile.is_zipfile(fileloc):
+                with zipfile.ZipFile(fileloc) as z:
+                    dag_filelocs.extend(

Review comment:
       I wonder if this should be done once in `_refresh_dag_dir` instead of done twice here and in `remove_deleted_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

[GitHub] [airflow] ephraimbuddy merged pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #18243:
URL: https://github.com/apache/airflow/pull/18243


   


-- 
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] uranusjr edited a comment on pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#issuecomment-919699445


   I think this needs a rebase. Not sure if the `max_retries` failures are related; don’t feel like it, but I don’t see anything in main modifying that recently either.
   
   ```
    tests/models/test_cleartasks.py::TestClearTasks::test_clear_task_instances: assert 3 == 1
    +  where 3 = <TaskInstance: test_clear_task_instances.1 scheduled__2016-01-01T00:00:00+00:00 [None]>.max_tries
   tests/models/test_cleartasks.py::TestClearTasks::test_clear_task_instances_with_task_reschedule: AssertionError: assert 0 == 1
    +  where 0 = <function TestClearTasks.test_clear_task_instances_with_task_reschedule.<locals>.count_task_reschedule at 0x7f49a824fd40>('1')
    +    where '1' = <TaskInstance: test_clear_task_instances_with_task_reschedule.1 scheduled__2016-01-01T00:00:00+00:00 [up_for_reschedule]>.task_id
   tests/models/test_cleartasks.py::TestClearTasks::test_dag_clear: assert 3 == 1
    +  where 3 = <TaskInstance: test_dag_clear.test_dag_clear_task_1 scheduled__2016-01-01T00:00:00+00:00 [None]>.max_tries
   tests/models/test_cleartasks.py::TestClearTasks::test_operator_clear: assert 1 == 0
    +  where 1 = <TaskInstance: test_operator_clear.dummy_op scheduled__2016-01-01T00:00:00+00:00 [success]>.max_tries
   ```


-- 
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] uranusjr commented on pull request #18243: Fix deleting of zipped Dags in Serialized Dag Table

Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18243:
URL: https://github.com/apache/airflow/pull/18243#issuecomment-919699445


   I think this needs a rebase. Not sure if the `max_retries` failures are related; don’t feel like it, but I don’t see anything in main modifying that recently either.


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