Dev-iL commented on PR #61702:
URL: https://github.com/apache/airflow/pull/61702#issuecomment-3932241676
Here's what Opus had to say:
----
## PR Review: Deadline UUID Reuse on Re-serialization
**PR Goal:** When a DAG is re-serialized (e.g., during periodic parsing),
preserve existing deadline alert UUIDs if the deadline definitions haven't
changed, avoiding unnecessary `SerializedDagModel` hash changes and record
churn.
---
### AIP Alignment
| AIP Requirement | Status | Notes |
| --- | --- | --- |
| DAG-level deadlines only | OK | Changes are scoped to DAG-level
`DeadlineAlert` |
| Must not cause extra DAG file parsing | OK | Prevents unnecessary
SerializedDagModel recreations caused by UUID regeneration |
| Extensible for future task/dataset deadlines | OK | `matches_definition`
is a general-purpose comparison, no coupling to DAG-level specifics |
| Callbacks work with Notifier system | N/A | Not touched by this PR |
| OTel metrics from day one | N/A | Not touched by this PR |
---
### Issues
#### 1. `NotImplemented` from a non-dunder method — Medium
**File:** `deadline_alert.py:76-78`
```python
def matches_definition(self, other: DeadlineAlert) -> bool:
if not isinstance(other, DeadlineAlert):
return NotImplemented
```
`NotImplemented` is a truthy singleton intended for dunder comparison
methods (`__eq__`, `__lt__`, etc.), where the Python runtime intercepts it to
try the reflected operation. For a regular method:
- The return type annotation `-> bool` is a lie — callers using type
checkers get no warning.
- `if alert.matches_definition(non_alert):` silently evaluates to `True`.
- In `_try_reuse_deadline_uuids`, the only call site passes two
`DeadlineAlertModel` instances so this path isn't hit today, but it's a latent
trap.
**Suggestion:** Either `raise TypeError(...)` or simply `return False`. If
there's a reason to distinguish "not comparable" from "not equal", use
`Optional[bool]` and return `None`.
---
#### 2. Temporary ORM object with `id="temp"` — Medium
**File:** `serialized_dag.py:449-454`
```python
temp_alert = DeadlineAlertModel(
id="temp",
reference=deadline_data[DeadlineAlertFields.REFERENCE],
interval=deadline_data[DeadlineAlertFields.INTERVAL],
callback_def=deadline_data[DeadlineAlertFields.CALLBACK],
)
```
This constructs a SQLAlchemy model instance with a bogus primary key solely
to call `matches_definition`. Problems:
- If the `id` column gains UUID validation (or a DB-side default trigger),
this breaks.
- The object may be inadvertently added to the session by SQLAlchemy's
identity map depending on session configuration.
- It's coupling to internal knowledge that `matches_definition` ignores `id`.
**Suggestion:** Compare the raw fields directly without constructing a model
instance:
```python
def _definitions_match(deadline_data: dict, existing: DeadlineAlertModel) ->
bool:
return (
deadline_data[DeadlineAlertFields.REFERENCE] == existing.reference
and deadline_data[DeadlineAlertFields.INTERVAL] == existing.interval
and deadline_data[DeadlineAlertFields.CALLBACK] ==
existing.callback_def
)
```
This removes the need for `matches_definition` on the model entirely, or
alternatively keeps both but makes the temp-object path unnecessary.
---
#### 3. Unnecessary alert clear+recreate when non-deadline DAG attributes
change — Low
In `write_dag`, when reuse succeeds, `deadline_uuid_mapping` is set to the
(non-empty) reuse dict:
```python
deadline_uuid_mapping = cls._try_reuse_deadline_uuids(...)
if deadline_uuid_mapping is not None:
dag.data["dag"]["deadline"] = existing_deadline_uuids
```
Later, in the UPDATE branch:
```python
if deadline_uuid_mapping:
updated_serialized_dag.deadline_alerts.clear()
cls._create_deadline_alert_records(updated_serialized_dag,
deadline_uuid_mapping)
```
If deadlines are unchanged but another DAG attribute changes (e.g., a task
is added), the hash changes, the UPDATE succeeds, and the code deletes and
recreates identical deadline alert rows — a pointless write cycle.
**Fix:** After successful reuse, set `deadline_uuid_mapping = {}`:
```python
if deadline_uuid_mapping is not None:
dag.data["dag"]["deadline"] = existing_deadline_uuids
deadline_uuid_mapping = {} # alerts already exist, nothing to create
```
---
#### 4. All-or-nothing UUID regeneration — Low / Design Note
`_try_reuse_deadline_uuids` returns `None` (regenerate **all** UUIDs) if
**any single** deadline definition changed. For a DAG with 3 deadlines where
only one changed, the 2 unchanged deadlines also get new UUIDs and new DB rows.
Given the AIP's test matrix shows 1–3 deadlines per DAG, this is pragmatic.
But if task-level deadlines are added (AIP future scope), the count could grow.
Worth a `# TODO` noting the trade-off.
---
### Test Coverage
| Scenario | Covered | File |
| --- | --- | --- |
| Re-serialization preserves UUIDs | Yes |
`test_dag.py:test_dag_with_multiple_deadlines` |
| Interval change triggers new SerDag + new alert | Yes |
`test_serialized_dag.py:test_deadline_interval_change_triggers_new_serdag` |
| `matches_definition` — equal/different/wrong-type | Yes |
`test_deadline_alert.py:test_deadline_alert_matches_definition` |
| Reference change triggers new UUIDs | **No** | — |
| Callback change triggers new UUIDs | **No** | — |
| Non-deadline DAG change with unchanged deadlines | **No** | — |
| Count mismatch (add/remove a deadline) | **No** | — |
The last four are exercised implicitly by `_try_reuse_deadline_uuids`'s
logic but not explicitly tested. The reference and callback change cases are
the most important gaps — if `matches_definition` regresses on those fields,
only the unit test catches it, not an integration test.
---
### Summary
The core approach is sound — comparing deadline definitions to decide
whether to reuse UUIDs is the right fix for the hash-churn problem. The main
concerns are the `NotImplemented` return from a regular method (latent
truthy-trap), the temporary ORM object construction (fragile), and a minor
inefficiency in the update path. Test coverage for the happy path is solid;
edge-case integration tests could be stronger.
--
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]
