ferruzzi commented on code in PR #68961:
URL: https://github.com/apache/airflow/pull/68961#discussion_r4170701328
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml:
##########
@@ -3643,6 +3643,9 @@ components:
description: Interval in seconds between the reference time and the
deadline.
Null for a dynamic interval (e.g. a VariableInterval) whose value
is only
resolved at scheduler evaluation time.
+ fire_on_failure:
Review Comment:
Sorry, I was out sick most of the week and just catching up. Here's my
opinion and my reasoning. My intent was fail-early, I would like to drop the
flag and call this a bug-fix. But that is not how the feature is documented.
The doc pages say (some quotes, not exhaustive):
- "This example will send an email notification if the Dag hasn't finished
30 minutes after it was queued." (Core Concepts)
- "would trigger the alert if the Dag hasn't completed 15 minutes after it
was scheduled to start, regardless of when (or if) it actually began
executing." (how-to, DAGRUN_LOGICAL_DATE)
- "Alert if 30 minutes past average runtime", "Dag is running longer than
expected!" (how-to, AVERAGE_RUNTIME)
Which means a user going off of only what they can see in the docs would
expect that failure is a "finished" state and the callback shouldn't fire.
That's what the Issue claims, and it's a fair reading of the docs.
To test this, I set Claude a task: "There is some interesting discussion
around the Deadline Alerts feature in Airflow. I want you to answer this
question using ONLY official public airflow doc pages, do not use my design
notes, code comments, open github issues, web searches, etc. I want a blind
reading direct from official documentation as a new user might have, not as an
insider with design and implementation knowledge. If I create a Dag with a
DeadlineAlert whose interval is three hours and that Dag fails after ten
minutes, what is the expected behavior? Would the callback fire after 10
minutes, 3 hours, or never? Form your answer in the format `Setting a Deadline
Alert could be read as "If this Dag _____ before this time, then fire this
callback"`.
and it replied with: "If this Dag has not finished before this time, then
fire this callback"` It would never fire. That is the only one of the three a
doc-only reader can reasonably land on.
Which implies that regardless of my intention, the documentation as written
doesn't convey what I meant for it to say.
If we rephrase the docs to say "if this Dag has not **succeeded** before
this time, then do this callback", this is closer to what I had in mind and
what Shivaam implemented. I'd say that was a bug-fix toward my vision, but if
we're being fair, it's moving away from the documented behavior and the docs
should be rephrased accordingly from "finished" to "succeeded", etc. But,
regardless of my intention, I think I have to admit that a fair reading of the
docs would be "do not fire on failure" which makes it hard to argue that this
is a bug-fix and why I backed the flag.
Now, If we're open to changing the default behavior, then I'd actually
propose inverting the flag. Make it work as-intended (fire on fail by default)
with an opt-out (treat failure as "finished"; prune on failure and do not fire
the callback). Users could then combine Deadlines with `on_failure_callback`
or other methods of monitoring Dag failures to distinguish between "ran long"
and "failed", and they don't necessarily want duplicate alarms on failure.
Which I think is the use-case that caused me to implement it this way in the
first place, but now it's just causing confusion.
--
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]
