> > 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 > >
