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