ferruzzi commented on PR #68961: URL: https://github.com/apache/airflow/pull/68961#issuecomment-4812887549
Thanks for the PR and the thorough testing. This had a pretty long discussion in the Issue and I still don't think short-circuiting by default is the right answer. Let's say you have a Dag which generates a report which you need for your morning meeting so you set a Deadline Alert for 9AM which emails you saying your report won't be ready. Case 1) You work at Big Company. The Dag fails at 7AM, an admin sees this, determines it's just a network hiccup and clears the failing tasks which then pass at 7:30. If the deadline fired on failure, you would have your report **and** a notice saying you won't. Case 2) Your Dag may have an `on_failure_callback` configured which notifies you of the failure. If the deadline also fires immediately, you now get two notifications about the same event with different messaging: Schrodinger's Dag is both failed and late. Case 3) You work at Small Company and don't have an admin team, you do it all yourself, so a Dag failing likely won't be cleared and the missed deadline is inevitable. I'd argue that `on_failure_callback` currently handles this, but I can see the advantage of bundling it into the Deadline Alert directly.. Case 4) You realize something is wrong with the input and you manually mark the Dag as FAILED so the report doesn't get generated with bad data. Are you going to fix the data and re-trigger the Dag, or is that the end of the story? We don't know and shouldn't guess. Worth noting: this PR wouldn't cover this case either, which would introduce a difference in behavior between an automatic failure and a manual one. Overall, always short-circuiting is not the answer. I would propose three alternatives which are not mutually exclusive: 1. Add a flag to the Deadline Alert class. Naming is hard, but something like `fire_on_failure: bool = False` and let the user decide depending on their use case. 2. Add a hook in the Deadline class which accepts a `run_id` and prunes all future deadlines for that run. This would allow `on_failure_callback` (or any other callback) to take action and then call the new hook to remove the now-redundant deadline(s) so you don't get double-notified. 3. Anywhere in the UI where a user can mark a Dag as FAILED should have an option to also clear related Deadlines. To be clear, I do agree that there is a real user need here, I'm not trying to kill this or dissuade you. I just want to make sure we solve it without breaking the existing use cases. It's one very valid use case, but that doesn't invalidate the others. That said, here's my suggestion: I'd also ask that the `callback.data` persistence fixes in `deadline.py` be split into a separate PR since those fix a real bug independent of this feature question. Whatever we do about the failure state, that looks like an actual legit improvement/bug-fix. Then I'd suggest reworking this PR to implement the flag and tracking the other two options as new Issues. ((tagging @ramitkataria since he was also involved in the original discussion and may have thoughts.)) -- 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]
