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