Hey anyone from, the BIG users, any comments here ? Any thoughts about the operation side of this? I am not sure if my worries are too "excessive", so I would love to hear your thoughts here.. I think it would be great to unblock Andrew so that he can carry on with the AIP (which I think is great feature BTW).
I think this boils down to two questions: 1) Would you be worried by having a single thread running all such triggers for the whole installation where users could potentially develop their own "blocking" triggers by mistake and block others ? 2) What kind of monitoring/detection/prevention would you like to see to get it "under control" :) J, On Tue, Apr 27, 2021 at 9:04 PM Andrew Godwin <andrew.god...@astronomer.io.invalid> wrote: > > Oh totally, I was partially wearing my SRE hat when writing up parts of this > (hence why HA is built-in from the start), but I'm more used to running web > workloads than Airflow, so any input from people who've run large Airflow > installs are welcome. > > > That will not work I am afraid :). If we open it up for users to use, we > > have to deal with consequences. We have to be prepared for people doing all > > kinds of weird things - at least this is what I've learned during the last > > 2 years of working on Airflow. > > It's maybe one of the key lessons I've had from 15 years writing open source > - people will always use your hidden APIs no matter what! > > I think in this case, it's a situation where we just have to make it > progressively safer as you go up the stack - if you're just using Operators > you are fine, if you are authoring Operators there's some new things to think > about if you want to go deferrable, and if you're authoring Triggers then you > need a bit of async experience. Plus, people who are just writing standard > non-deferrable operators have nothing to worry about as this is purely an > additive feature, which I like; letting people opt-in to complexity is always > nice. > > Hopefully we can get enough safety guards in that all three of these levels > will be reasonably accessible without encouraging people to go straight to > Triggers; it would likely be an improvement over Smart Sensors, which as far > as I can tell lack a lot of them. > > Andrew > > On Tue, Apr 27, 2021 at 12:29 PM Jarek Potiuk <ja...@potiuk.com> wrote: >> >> Hey Andrew, >> >> Just don't get me wrong :). I love the idea/AIP proposal. Just want to >> make sure that from day one we think about operational aspects of it. >> I think the easier we make it for the operations people, the less we >> will have to deal with their problems in the devlist/Github issues. >> >> I would love to hear what others think about it - maybe some folks >> from the devlist who operate Airflow in scale could chime in here - we >> have some people from AirBnB/Twitter etc... Maybe you could state >> expectations when it comes to the operational side if you'd have 1000s >> of Triggers that potentially interfere with each other :) ? >> >> >> >> On Mon, Apr 26, 2021 at 9:21 PM Andrew Godwin >> <andrew.god...@astronomer.io.invalid> wrote: >> > 1) In general, I'm envisioning Triggers as a thing that are generally >> > abstracted away from DAG authors - instead, they would come in a provider >> > package (or core Airflow) and so we would be expecting the same higher >> > level of quality and testing. >> >> I see the intention, but we would have to have pretty comprehensive >> set of triggers from day one - similar to the different types of "rich >> intervals" we have in >> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-39+Richer+scheduler_interval. >> Maybe a list (groups) of the triggers we would like to have as >> "built-ins" would be really helpful? >> But even then, I think the "extendability" of this solution is what I >> like about it. More often than not, users will find out that they need >> something custom. I think if we describe the interface that the >> Trigger should implement and make it a "first-class" citizen - >> similarly as all other concepts in Airflow, it is expected that users >> will override them - Operators, Sensors, even the new "Richer >> schedules" are "meant" to be implemented by the users. If they are >> not, we should not make it public but rather have (and accept) only a >> fixed set of those - and for that we could just implement an Enum of >> available Triggers. By providing an interface, we invite our users to >> implement their own custom Triggers, and I think we need to deal with >> consequences. I think we have to think about what happens when users >> start writing their triggers and what "toolbox" we give the people >> operating Airflow to deal with that. >> >> > a) I do think it should all be fixed as asyncio, mostly because the >> > library support is there and you don't suddenly want to make people have >> > to run multiple "flavours" of triggerer process based on how many triggers >> > they're using and what runtime loops they demand. If trio were more >> > popular, I'd pick that as it's safer and (in my opinion) better-designed, >> > but we are unfortunately nowhere near that, and in addition this is not >> > something that is impossible to add in at a later stage. >> >> Asyncio would also be my choice by far so we do not differ here :) >> >> From what I understand, you propose a single event loop to run all the >> deferred tasks? Am I right? >> My point is that multiple event loops or Thread-based async running >> are also part of asyncio. No problem with that. Asyncio by default >> uses a single event loop, but there is no problem to use more. This >> would be quite similar to "queues" we currently have with celery >> workers. I am not telling we should, but I can see the case where this >> might be advisable (for example to isolate groups of the deferred >> tasks so that they do not interfere/delay the other group). What do >> you think? >> >> > b) There's definitely some hooks we can use to detect long-running >> > triggers, and I'll see if I can grab some of them and implement them. At >> > very least, there's the SIGALRM watchdog method, and I believe it's >> > possible to put nice timeouts around asyncio tasks, which we could use to >> > enforce a max runtime specified in the airflow.cfg file. >> >> I agree it will be indeed difficult to prevent people from making >> mistakes, but we should really think about how we should help the >> operations people to detect and diagnose them. And I think it should >> be part of specification: >> >> 1) what kind of metrics we are logging for the triggers - I think we >> should gather and publish (using airflow's metrics system) some useful >> metrics for the operations people (number of executions/execution >> length, queuing/delay time vs. expectation for some trigger like date >> trigger) etc. for all triggers from day one. >> 2) you mentioned it - maybe we should have Debug mode turned on by >> default: https://docs.python.org/3/library/asyncio-dev.html#debug-mode >> . It has some useful features, mainly automated logging of too long >> running async methods. Not sure what consequences it has though. >> 3) max execution time would be even nicer indeed >> 4) maybe there should be some exposure in the UI/CLI/API on what's >> going in with triggers? >> 5) maybe there should be a way to cancel /disable some triggers that >> are mis-behaving - using the UI/CLI/API - until the code gets fixed >> for those ? >> >> I think we simply need to document those aspects in "operations" >> chapter of your proposal. I am happy to propose some draft changes in >> the AIP if you would like to, after we discuss it here. >> >> > 2) This is why there's no actual _state_ that is persisted - instead, you >> > pass the method you want to call next and its keyword arguments. Obviously >> > we'll need to be quite clear about this in the docs, but I feel this is >> > better than persisting _some_ state. Again, though, this is an >> > implementation detail that would likely be hidden inside an Operator or >> > Sensor from the average DAG user; I'm not proposing we expose this to >> > things like PythonOperator or TaskFlow yet, for the reasons you describe. >> > I wish I had a better API to present for Operator authors, but with the >> > fundamental fact that the Operator/Task is going to run on different >> > machines for different phases of its life, I think having explicit "give >> > me this when you revive me" arguments is the best tradeoff we can go for. >> >> Agree. We cannot do much about it. I think we should just be very >> clear when we document the life cycle. Maybe we should even update the >> semantics of pre/post so that "pre_execute()" and "post_execute()" >> are executed for EVERY execute - including the deferred one (also >> execute_complete()). This way (in your example) the task execution >> would look like : pre_execute(), execute(-> throw TaskDeferred()), >> post_execute() and then on another worker pre_execute(), >> execute_complete(), post_execute(). I think that would make sense. >> What do you think? >> >> > 3) I do think we should limit the size of the payload, as well as the >> > kwargs that pass between deferred phases of the task instance - something >> > pretty meaty, like 500KB, would seem reasonable to me. I've also run into >> > the problem in the past that if you design a messaging system without a >> > limit, people _will_ push the size of the things sent up to >> > eyebrow-raisingly-large sizes. >> >> Yeah. I think XCom should be our benchmark. Currently we have >> MAX_XCOM_SIZE = 49344. Should we use the same? >> >> And that leads me to another question, actually very important. I >> think (correct me please if I am wrong) it is missing in the current >> specs. Where the kwargs are going to be stored while the task is >> deferred? Are they only stored in memory of the triggerer? Or in the >> DB? And what are the consequences? What happens when the tasks are >> triggered and the triggerer is restarted? How do we recover? Does it >> mean that all the tasks that are deferred will have to be restarted? >> How? Could you please elaborate a bit on that (and I think also it >> needs a chapter in the specification). >> >> > Overall, I think it's important to stress that the average DAG author >> > should not even know that Triggers really exist; instead, they should just >> > be able to switch Sensors or Operators (e.g. DateTimeSensor -> >> > AsyncDateTimeSensor in my prototype) and get the benefits of deferred >> > operators with no extra thought required. >> >> That will not work I am afraid :). If we open it up for users to use, >> we have to deal with consequences. We have to be prepared for people >> doing all kinds of weird things - at least this is what I've learned >> during the last 2 years of working on Airflow. See the comment about. >> If we do not want users to implement custom versions of triggers we >> should use Enums and a closed set of those. If we create an API an >> interface - people will use it and create their own, no matter if we >> want or not. >> >> >> J. -- +48 660 796 129
