kaxil commented on code in PR #68947:
URL: https://github.com/apache/airflow/pull/68947#discussion_r3703006643
##########
airflow-core/tests/unit/models/test_serialized_dag.py:
##########
@@ -875,10 +875,12 @@ def __init__(self, *, task_id: str, **kwargs):
# Hashes should be identical
assert hash_1 == hash_2, "Hashes should be identical when dicts are
sorted consistently"
- def test_dynamic_dag_update_preserves_null_check(self, dag_maker, session):
+ def
test_new_dag_version_is_created_when_version_exists_but_serialized_dag_row_missing(
+ self, dag_maker, session
+ ):
"""
Test that dynamic DAG update gracefully handles case where
SerializedDagModel doesn't exist.
Review Comment:
Docstring still describes the old behavior. "gracefully handles" was the
euphemism for returning False, and "dynamic DAG update" isn't the scenario (a
missing serialized row is). Something like "A new DagVersion with a serialized
row is created when the latest version has no serialized_dag row" would match
the new assertions. Same for the comment on line 911, which reads "Try to
update" then "should create".
##########
airflow-core/tests/unit/models/test_serialized_dag.py:
##########
@@ -906,16 +908,19 @@ def test_dynamic_dag_update_preserves_null_check(self,
dag_maker, session):
# Verify no SerializedDagModel exists
assert SDM.get("test_missing_serdag", session=session) is None
- # Try to update - should return False gracefully (not crash)
+ # Try to update - should create the serialized dag
result = SDM.write_dag(
dag=lazy_dag,
bundle_name="test_bundle",
bundle_version=None,
min_update_interval=None,
session=session,
)
+ session.commit()
- assert result is False # Should return False when SerializedDagModel
is missing
+ serialized_dag = session.scalar(select(SDM).where(SDM.dag_id ==
"test_missing_serdag").limit(1))
+ assert serialized_dag is not None
Review Comment:
This asserts that some row exists for the dag_id, but the property that
makes the dag schedulable again is that the row belongs to the latest
DagVersion. `SDM.get("test_missing_serdag", session=session)` already orders by
`version_number`, so using it plus an assert on `version_number` would pin what
the test name claims. As written the test still passes in the case where an
older version holds the row and the latest one doesn't.
Worth keeping a case for the `rowcount == 0` branch too. After this change
nothing in the file exercises it, and if a later cleanup drops it as dead code
that path would run the dag_version and dag_code updates and return True having
written no serialized row.
##########
airflow-core/src/airflow/models/serialized_dag.py:
##########
@@ -723,7 +723,7 @@ def write_dag(
)
)
- if dag_version and not has_task_instances:
+ if dag_version and not has_task_instances and serialized_dag_hash is
not None:
Review Comment:
`serialized_dag_hash is not None` isn't the same predicate as "the latest
dag_version has a serialized row". `dag_hash` is `nullable=False` and `sd_subq`
in `_prefetch_dag_write_metadata` inner-joins `serialized_dag` to
`dag_version`, so the hash comes from the highest version that still has a row,
while `dag_version` here is the absolute latest. If only the latest version's
row is missing, the hash is non-None, this guard passes, the UPDATE matches 0
rows and we return False at line 747 again.
Checked on this branch in breeze: v1 with a serialized row plus a bare
latest v2 keeps returning False, with no row ever created for v2. If the
incoming hash matches v1's, it returns even earlier, at line 710 on the
unchanged-hash short-circuit. Since the only production creator of a
`dag_version` row is line 769, in the same transaction as the insert at line
784, both shapes come out of the same accident class.
Pairing the prefetched hash with the version this branch actually updates
covers both (outerjoin `serialized_dag` off the latest-version subquery so
`dag_hash` is None whenever the latest version has no row). I tried that in a
probe and both shapes then heal on the next parse. Letting `rowcount == 0` fall
through to `DagVersion.write_dag` is a good complement since it's the direct
signal, but on its own it misses the unchanged-hash case, and it leaves
`min_update_interval` reading the older row's `last_updated`. If the clause
does stay here, a line of comment saying what it detects would help, since as
written it reads as null-safety on a non-nullable column.
--
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]
