Just to make it clear I marked this PR as Draft - and I would really appreciate some comments about the approach and direction (in the light of Connexion 2 being dead-end holding us from security-related upgrades and generally holding us back).
Surely - there are things to be improved in this PR, and we already got quite a deal of comments there (and yes, it's quite obvious it's not mergeable in the current state - I made a number of comments and TODOs there, but likely there will be more - there are already some). And if we feel, we need a separate AIP for that one - we can definitely think about writing and approving one - describing in more detail the architectural changes introduced (which as I understand it, is mostly following best directions of where Python apps of the kind are heading towards with WSGI => ASGI move for quite a long time already). But mostly it's a call for help from those who **could** help and move it into a "PROD Ready" solution, if we think it's a good direction (and possibly help to write the AIP). Forming a small task force of people who would like to make it "good to go" - people who know what they are doing about such change, and can bring some more "testing" capacity - especially from those who host Airflow as managed services. Or maybe that could be one of the "internal" changes for the elusive "Airflow 3" we've been talking about for such a long time but we were never able to define what it is going to be? Dunno what others think :). But it would be good to hear what others think, as this one is one of the things that definitely holds us back. J. On Tue, Apr 16, 2024 at 10:36 PM Jarek Potiuk <ja...@potiuk.com> wrote: > I don't think AIP is needed (because it's not really a user facing change > and generally the change is backwards compatible. > > But yeah - the call for help is mostly to see / review / discuss it when > we really know the scope of the change after we have a PR that proves that > yes - it can be done. > > It might indeed need voting when we get to the act of considering merging > the change, but I am not sure if we need AIP describing it. We probably > would not be able to write the AIP upfront - before attempting to migrate > it to be honest. > > I think what I am really looking for is to achieve the level of confidence > that might let us decide "yep it's good to go" (hopefully). The important > thing here is while the stack under-the-hood is changing a lot, the actual > number of changes in airflow code (except the tests) is rather small - > mostly initialization and wiring the stack together. > > We do not have to merge it now or even soon or maybe even never (though > Connexion 2 is a dead-end and sooner or later we will have to replace it > with **something** when it comes to serving our API). > Connexion 3 seems to be a good - and natural - candidate . > > I think with this change in this stage - we more or less know what > it really means to migrate to Connexion 3 (additionally with having some > comments from the Connexion maintainer who already looks in more details at > the comments and questions in the PR) I think it's where we can see if that > is something we want to move forward with. And especially if we will > have feedback from those who know more about the involved technology stack. > > That's why I mostly opened this thread :) > > J. > > > On Tue, Apr 16, 2024 at 5:15 PM Ryan Hatter > <ryan.hat...@astronomer.io.invalid> wrote: > >> Does the scope of this PR warrant an AIP? >> >> On Tue, Apr 16, 2024 at 6:40 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> >> > Hello here, >> > >> > I have a kind request for help from maintainers (and other contributors >> who >> > are not maintainers) - on the Connexion 3 migration for Airflow. PR here >> > (unfortunately - it's one big PR and cannot be split): >> > https://github.com/apache/airflow/pull/39055. >> > >> > I would love some general comments on this - especially from those who >> are >> > more experts than me on those web frameworks - is it safe and ok to >> > migrate, do we need to do some more testing on that? What do other >> > maintainers think? >> > >> > This is not a "simple" change - it introduces a pretty fundamental >> change >> > in how our web app is handled - It changes from WSGI to ASGI interface >> > (though we use gunicorn as WSGI). But it's also absolutely needed - >> > because we already had some security issues connected with old >> > dependencies (Werkzeug) - raised - and Connexion 3 migration seems to be >> > the easiest way to get to the latest, maintained versions of the >> > dependencies. >> > >> > That's why I'd really like a few more maintainers - and people from the >> > Astronomer, Google and AWS to take it for a spin and help to test that >> > change and say "yep. It looks good, we can merge it". I would >> especially >> > appreciate some more "scale" testing on it. It seems that performance >> and >> > resource usage is not affected and ASGI interface and uvicorn should >> nicely >> > replace all the different worker types we could have for gunicorn - but >> I >> > would love to have confirmation for that. >> > >> > The PR has been started by Vlada and Maks from the Google team - and >> > with the help of Sudipto and Satoshi - two interns from Major League >> > Hacking - supported by Royal Bank of Canada - finally we have a stable, >> > working version and green PR. Airflow webserver + API seems to work >> well, >> > stable (and generally back-compatible) on both - development (local + >> > breeze) and PROD image. >> > >> > I took a mentorship and leading role on it - but personally I have been >> > learning on the go about WSGI/ASGI and all changes needed - I am not an >> > expert at all in those. We followed the directions from Connexion'\s >> > maintainer Robb Snyders - and I asked him to help and comment on the PR >> in >> > a number of places - but that's why I need more help and experts' eyes >> and >> > hands to be quite sure it can be safely nerged. >> > >> > I extracted it and squashed more than 100 commits on it into a single >> one >> > to make it easier to start new conversations. >> > >> > Once again PR here: https://github.com/apache/airflow/pull/39055 >> > >> > Also - we need to decide when is the best time to merge the PR - it does >> > not introduce a lot of changes in the code of the app, but it changes a >> lot >> > of test code to make it compatible with Startlette test client - we can >> > continue rebasing it and fixing new changes for a short while - but I >> think >> > the sooner we migrate it - the better - it will give more time for >> testing >> > in the future MINOR airflow version. >> > >> > J. >> > >> >
