kaxil commented on code in PR #70586:
URL: https://github.com/apache/airflow/pull/70586#discussion_r3707293520
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -477,10 +477,11 @@ def deactivate_stale_dags(
)
to_deactivate.add(dag.dag_id)
continue
- # When the Dag's last_parsed_time is more than the
stale_dag_threshold older than the
- # Dag file's last_finish_time, the Dag is considered stale as has
apparently been removed from the file,
- # This is especially relevant for Dag files that generate Dags in
a dynamic manner.
- file_info = DagFileInfo(rel_path=Path(dag.relative_fileloc),
bundle_name=dag.bundle_name)
+ rel_path = Path(dag.relative_fileloc)
+ file_info = DagFileInfo(rel_path=rel_path,
bundle_name=dag.bundle_name)
+ if file_info not in last_parsed:
+ # Zip-packaged dags are keyed by the archive path, not the
inner file, so try the parent as well
Review Comment:
Since this hunk now conflicts with main (#63185 rewrote it and kept these
lines), can the rebase also restore the original three-line comment explaining
the staleness heuristic? The threshold comparison below is still the
non-obvious part, and the new comment only covers the zip keying.
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -1046,6 +1044,72 @@ def test_scan_stale_dags(self, session):
# SerializedDagModel gives history about Dags
assert serialized_dag_count == 1
+ @pytest.mark.usefixtures("testing_dag_bundle")
+ def test_scan_stale_dags_deactivates_zip_packaged_dags(self, session,
test_zip_path):
+ """
+ Ensure that zip-packaged DAGs are marked inactive when the file is
parsed but the
Review Comment:
Could the docstring say what this test pins down? Zip dags store
`relative_fileloc` as `<archive>.zip/<inner>.py` while `_file_stats` is keyed
by the archive path, so without the parent fallback the lookup misses and the
dag is never deactivated. As written it reads the same as the sibling test's
docstring.
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -477,10 +477,11 @@ def deactivate_stale_dags(
)
to_deactivate.add(dag.dag_id)
continue
- # When the Dag's last_parsed_time is more than the
stale_dag_threshold older than the
- # Dag file's last_finish_time, the Dag is considered stale as has
apparently been removed from the file,
- # This is especially relevant for Dag files that generate Dags in
a dynamic manner.
- file_info = DagFileInfo(rel_path=Path(dag.relative_fileloc),
bundle_name=dag.bundle_name)
+ rel_path = Path(dag.relative_fileloc)
+ file_info = DagFileInfo(rel_path=rel_path,
bundle_name=dag.bundle_name)
+ if file_info not in last_parsed:
+ # Zip-packaged dags are keyed by the archive path, not the
inner file, so try the parent as well
Review Comment:
On the PR body's idea of a property on the DAG model: +1 to giving this
mapping a name eventually. This lookup is the reverse of what
`_get_observed_filelocs` does for `deactivate_deleted_dags` (archive to inner
paths there, fileloc to parse-unit key here), and nothing links the two sites
today. If the importer's one-level rule (`_load_modules_from_zip` skips entries
with `len(zip_path.parts) > 1`) ever changes, the deletion path would keep
working while this fallback silently stops matching. A small helper or
`DagFileInfo` classmethod would keep them in step. Fine as a follow-up rather
than in this PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
