You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "michaelmicheal (via GitHub)" <gi...@apache.org> on 2023/02/07 19:28:57 UTC
[GitHub] [airflow] michaelmicheal opened a new pull request, #29407: Ensure Serialized DAG is deleted
michaelmicheal opened a new pull request, #29407:
URL: https://github.com/apache/airflow/pull/29407
dag processing manager, dag serialization delete, only filter on dag folder when dag processing is in a standalone processor
<!--
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/
-->
---
**^ 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 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
[GitHub] [airflow] michaelmicheal commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1428462338
@ephraimbuddy Any idea why I'm getting this error? Doesn't seem to be related to my changes
--
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] michaelmicheal commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1424742240
> Can you rebase? would like it to be green before merge
I've already rebased, is there a way to rerun CI?
--
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] michaelmicheal commented on a diff in pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on code in PR #29407:
URL: https://github.com/apache/airflow/pull/29407#discussion_r1100266205
##########
airflow/models/serialized_dag.py:
##########
@@ -251,7 +251,7 @@ def remove_deleted_dags(
cls.fileloc_hash.notin_(alive_fileloc_hashes),
cls.fileloc.notin_(alive_dag_filelocs),
or_(
- cls.processor_subdir is None,
+ cls.processor_subdir.is_(None),
Review Comment:
Agreed, I added a dummy `processor_subdir` into the test to ensure we rely on `cls.processor_subdir.is_(None)` correctly filtering. Let me know if that is 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
[GitHub] [airflow] ephraimbuddy commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1424706143
Can you rebase? would like it to be green before merge
--
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] michaelmicheal commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1424699959
@eladkal @bbovenzi is this good to merged?
--
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 #29407: Ensure Serialized DAG is deleted
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #29407:
URL: https://github.com/apache/airflow/pull/29407
--
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] michaelmicheal commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1424281455
@ephraimbuddy is this good to merge?
--
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 diff in pull request #29407: Ensure Serialized DAG is deleted
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29407:
URL: https://github.com/apache/airflow/pull/29407#discussion_r1100210403
##########
airflow/dag_processing/manager.py:
##########
@@ -765,9 +765,13 @@ def _refresh_dag_dir(self):
else:
dag_filelocs.append(fileloc)
+ processor_subdir = None
+ if self.standalone_dag_processor:
+ processor_subdir = self.get_dag_directory()
+
SerializedDagModel.remove_deleted_dags(
alive_dag_filelocs=dag_filelocs,
- processor_subdir=self.get_dag_directory(),
+ processor_subdir=processor_subdir,
Review Comment:
Can you add tests to prove this?
--
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] potiuk commented on pull request #29407: Ensure Serialized DAG is deleted
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29407:
URL: https://github.com/apache/airflow/pull/29407#issuecomment-1428512704
Looks like temporary inability to fetch something. Likely transient error wiht some networking. Re-running 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] michaelmicheal commented on a diff in pull request #29407: Ensure Serialized DAG is deleted
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on code in PR #29407:
URL: https://github.com/apache/airflow/pull/29407#discussion_r1100230887
##########
airflow/dag_processing/manager.py:
##########
@@ -765,9 +765,13 @@ def _refresh_dag_dir(self):
else:
dag_filelocs.append(fileloc)
+ processor_subdir = None
+ if self.standalone_dag_processor:
+ processor_subdir = self.get_dag_directory()
+
SerializedDagModel.remove_deleted_dags(
alive_dag_filelocs=dag_filelocs,
- processor_subdir=self.get_dag_directory(),
+ processor_subdir=processor_subdir,
Review Comment:
While that worked, I was misunderstanding the `remove_deleted_dags` method and found out it was a bug in that method. See latest commit
--
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 diff in pull request #29407: Ensure Serialized DAG is deleted
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29407:
URL: https://github.com/apache/airflow/pull/29407#discussion_r1100257482
##########
airflow/models/serialized_dag.py:
##########
@@ -251,7 +251,7 @@ def remove_deleted_dags(
cls.fileloc_hash.notin_(alive_fileloc_hashes),
cls.fileloc.notin_(alive_dag_filelocs),
or_(
- cls.processor_subdir is None,
+ cls.processor_subdir.is_(None),
Review Comment:
While I know `is_` is the correct thing here, I think this gives us an opportunity to add an additional test to ensure we don't break it in the future
--
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