I think teardown should always happen no matter the short cricuit. Short circuit IMHO should be updated so that it should not be able to skip teardown.
Also I think Dennis later point about preserving the output / intermediate resources is sometimes important, but this is a debugging technique that might require some changes in the DAG. And here where I think the new trigger is cool - simply comment the @teardown in your dag and .... you get "don't remove resources on failure" effectively :) On Mon, Mar 27, 2023 at 6:43 PM Daniel Standish <daniel.stand...@astronomer.io.invalid> wrote: > > > > > 1) I am not sure if we should make it private (I am not even sure what > > it would mean to be private:) ). But If it means that setting the rule > > type for non-teardown task should raise an error (and of course > > documenting this rule as only (and automatically) being applied to > > teardown task - I am good with it. > > > Yeah I have no concerns with throwing error if this rule is used "manually" > by user. Like, at init time though, keeping it simple. I.e. I think we > shouldn't boil the ocean to prevent a creative user from finding a way to > use it in some weird way. > > 2) Yes. The teardown must run in any case. I think documenting the > > behaviour in ShortCircuit as a special case and - especially - adding > > some examples showing that would be enough > > > Clarify Jarek, are you saying that we should allow ShortCircuit to "short > circuit" the teardowns [either by default or perhaps optionally with a > skip_teardowns_too param] and document with examples? Or are you saying > shortcircuit operator should be updated so it's not possible to use it to > skip a teardown? > > > > > > > > > > On Mon, Mar 27, 2023 at 12:30 AM Jarek Potiuk <ja...@potiuk.com> wrote: > > > My view: > > > > 1) I am not sure if we should make it private (I am not even sure what > > it would mean to be private:) ). But If it means that setting the rule > > type for non-teardown task should raise an error (and of course > > documenting this rule as only (and automatically) being applied to > > teardown task - I am good with it. > > > > 2) Yes. The teardown must run in any case. I think documenting the > > behaviour in ShortCircuit as a special case and - especially - adding > > some examples showing that would be enough > > > > And yeah while it is indeed an implementation detail, it is somewhat > > "visible" as the public API is being concerned. So no wonder it came > > as a bit of a surprise while implementing setup/teardown (happens that > > something we consider as a detail for those deeply involved is a bit > > of a surprise for those that came to look at it at review time). I > > guess this is somewhat a consequence of the way we operate in a > > distributed environment and some details are being discussed in > > smaller groups that are more focused on getting things done (we had a > > few of those for AIP-44 for one). > > > > But eventually we are discussing it now, so I think it is cool. > > > > J. > > > > On Mon, Mar 27, 2023 at 8:43 AM Ash Berlin-Taylor <a...@apache.org> wrote: > > > > > > If the set-up ran then the tear down _must_ run. No question. > > > > > > Nothing should be able to change this fact. If you can, then they don't > > fulfill the stated purpose of tear down tasks in the AIP: to tidy up > > resources created by a set up task. > > > > > > On 27 March 2023 06:22:47 BST, Daniel Standish > > <daniel.stand...@astronomer.io.INVALID> wrote: > > > >> > > > >> When user set setup/teardown he has no idea unique trigger rule is set > > > >> under the hood. The user also has no idea that trigger rules are even > > > >> involved. That is not something he sees unless he checks the code of > > > >> teardown and setup decorators. > > > > > > > >This means that users of ShortCircuitOperator will not even know they > > need > > > >> to take action (until it wont work as expexted) and they will > > propbably > > > >> start as asking questions. > > > > > > > > > > > >Yeah, short circuit operator is a special operator that, if you're > > going to > > > >use it, you ought to know how it works. And we can easily add a note in > > > >the docs that emphasizes its behavior on this point. But I should point > > > >out, the same would be true if setup and teardown were not implemented > > with > > > >trigger rules; unless you investigate, read the docs, or look at the > > code, > > > >you would not know whether teardowns would be skipped by short > > circuit. So > > > >either way it's something that we'd have to lean on docs for. It's just > > > >not something that a prudent user would just make an assumption about. > > > > > > > >If the set-up ran then the short circuit shouldn't be able to skip it > > > >> > > > > > > > >I think this is overly and unnecessarily opinionated and limiting. I > > would > > > >not be concerned with having the default be one way or another, but to > > say > > > >that "you should not be able to skip it", I disagree with the notion > > that > > > >skipping a teardown with an operator specifically designed for the > > purpose > > > >should be *forbidden,* as your comment suggests. > > > > > > > >take for example creating a cluster - you still want to delete it at the > > > >> end even if you skipped all the other tasks! > > > >> > > > > > > > >Yeah that's a perfectly fine example. And, normally this is precisely > > how > > > >this feature works: if your task throws a skip exception, its downstream > > > >work tasks will be skipped but the teardown will run. But what we're > > > >talking about is a special operator whose unique purpose is to > > > >short circuit the dag. And as we know, every kind of dag you can > > imagine, > > > >there's a user who needs it. So someone will want to be able to skip a > > > >teardown. Teardowns aren't always going to be deleting a cluster. They > > > >might be deleting *files*, let's say. Well, what if you have this > > special > > > >operator in there to detect when something very bad happens and in that > > > >case you want to stop all processing and leave the current state alone > > so > > > >you can debug it. Is there any good reason to forbid this? I think no. > > > >As stewards of this pipeline writing language, we should have sensible > > > >defaults and maintain a good authoring interface, while allow users the > > > >power to write what they need. > > > > > > > > > > > > > > > > > > > >On Sun, Mar 26, 2023 at 9:19 PM Ash Berlin-Taylor <a...@apache.org> > > wrote: > > > > > > > >> If the set-up ran then the short circuit shouldn't be able to skip it: > > > >> take for example creating a cluster - you still want to delete it at > > the > > > >> end even if you skipped all the other tasks! > > > >> > > > >> This is precisely what I mean by set up and tear down tasks being > > special! > > > >> > > > >> On 27 March 2023 04:02:32 BST, Elad Kalif <elad...@apache.org> wrote: > > > >> >Thanks Daniel, > > > >> >Let me clarify my concern. > > > >> > > > > >> >When user set setup/teardown he has no idea unique trigger rule is > > set > > > >> >under the hood. The user also has no idea that trigger rules are even > > > >> >involved. That is not something he sees unless he checks the code of > > > >> >teardown and setup decorators. > > > >> > > > > >> >This means that users of ShortCircuitOperator will not even know > > they need > > > >> >to take action (until it wont work as expexted) and they will > > propbably > > > >> >start as asking questions. > > > >> > > > > >> >I'm not saying this should modify the plan just raising it as a > > potential > > > >> >source for pitfall. > > > >> > > > > >> >בתאריך יום ב׳, 27 במרץ 2023, 05:50, מאת Daniel Standish > > > >> ><daniel.stand...@astronomer.io.invalid>: > > > >> > > > > >> >> Thanks Elad for the feedback. > > > >> >> > > > >> >> re 1. i don't really see a problem with the trigger rule being > > public. > > > >> The > > > >> >> way I see it, it's another trigger rule like any other trigger > > rule. > > > >> Every > > > >> >> trigger rule behaves differently, that's true here too. This one > > > >> happens to > > > >> >> be relied upon for teardown tasks. That said, I don't think I > > would > > > >> >> necessarily be opposed to making it private. > > > >> >> > > > >> >> re 2, personally I kindof think it's a good thing. My > > understanding > > > >> from > > > >> >> your comments is that with ShortCircuitOperator you can set it to > > skip > > > >> all > > > >> >> downstream or just skip the direct relatives. To me this seems > > great > > > >> cus > > > >> >> it provides a way to either skip everything (including teardowns) > > or > > > >> just > > > >> >> the next task (thus potentially allowing teardowns to run). To me > > this > > > >> is > > > >> >> another way in which by staying within the existing dependency and > > > >> trigger > > > >> >> rule paradigm we have more consistent, predictable, and > > configurable > > > >> >> behavior. E.g. if we were not using normal deps and trigger > > rules, then > > > >> >> surely someone would have the opposite concern: "i want to use > > short > > > >> >> circuit operator to just skip all tasks including teardowns" and we > > > >> might > > > >> >> not be able to grant that wish, or at least not without more > > > >> development. > > > >> >> When you use an operator like this, you simply need to know what > > it does > > > >> >> and configure it in a manner appropriate for your use case. > > > >> >> > > > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org For additional commands, e-mail: dev-h...@airflow.apache.org
