Somehow James's comment went to a different thread --- merging it here @Leah Cole <[email protected]> -- Yes I will add them to the repo in a week or two, once everyone had time to read it here and discuss any thoughts/comments. ----
Valid concerns James. I think we need a good balance of both - "getting PRs merged in a timely fashion" and "making sure the PRs are thoroughly reviewed". There are some areas where I might be 100% sure of what is happening e.g DAG Serialization; then I might not need a second opinion but I might miss some scenarios when reviewing FAB related PRs. "Core Changes" for me is anything that touches task execution and its behaviour - Scheduler loop, Local Task jobs, related CLI commands, DAG Serialization. Unfortunately with FAB related changes, you are our only in-house expert, hence not many committers review those changes as they aren't sure *I think.* The gist of my suggestions is mainly that we have more eyes on PRs especially if they touch the "critical" path as it would be preferable to have a bit of delay (but not significant delays) in merging the PRs than merging it quickly without thoroughly reviewing (and possibly miss a bug). Regards, Kaxil On Fri, Nov 12, 2021 at 6:14 PM James Timmins <[email protected]> wrote: > These guidelines make sense generally, but I’m concerned a bit with the > requirement for two reviews for core changes. > > 1. It’s not clear to me what core changes are. Does this just mean > anything not related to providers? > 2. Airflow, to put it delicately, does not excel at getting changes > merged in a timely manner. I routinely find myself asking stakeholders > multiple times to review a change, and that’s before addressing any of > their feedback. I’m worried that doubling the approval requirement will > exacerbate one problem, without meaningfully solving the core issue of > changes getting released with bugs or breaking changes. There’s also a > psychological component to consider. If a PR has passed all tests and been > reviewed/approved by another committer, it’s far too easy for the second > reviewer to simply rubber-stamp without doing the kind of deep review > needed to catch bugs that were missed by the original programmer and the > first reviewer. > > Thanks, > James > On Sat, Nov 13, 2021 at 12:49 AM Leah Cole <[email protected]> wrote: > Regarding these guidelines, love em, how do y'all feel about having them > in the repo or somewhere in confluence? (maybe as part of a broader > maintainers guide?) > > On Thu, Nov 11, 2021 at 11:03 PM Jarek Potiuk <[email protected]> wrote: > >> Good guideline. All for it. Good point Ace, about automation. I think >> any "sustainable" guidelines that are going to survive and be followed >> in the future should be automated. >> >> I actually think we should introduce automation for those - especially >> that this seems entirely possible: >> >> 1) I believe it should be possible (and even easy) to add a check in >> CI that "2 reviews from committers are needed" when any of the core >> files change. Should be easy by the right "change set". We already >> have all the tools in our selective_chceks and notification to do >> that. We could even make a new "check" (similarly as we do >> "up-to-date" check for migrations" that will signal the status nicely >> if a core change has not been reviewed by two committers. >> >> 2) Similarly we could also similarly flag a code that does not change >> unit tests. This would also be a nice "signal" for new contributors, >> that they still have some work to do - and we could explain why in the >> message. That makes for a nice communication tool towards new >> contributors.. >> >> 3) Rests for migration raised by Ace- I think that is a good idea, but >> we would need to run it outside of the main CI, because the only >> reasonable tests that we can do if we run it or "biggish" amount of >> data. The problems with migrations are only really apparent when the >> amount of data is sizable. But maybe we could do that as an automated >> "pre-release" check - we just need a dump of a biggish database >> (mysql/postgres/mssql) and run the migration and measure the added >> migration time. >> >> J. >> >> On Fri, Nov 12, 2021 at 5:08 AM Ace Haidrey >> <[email protected]> wrote: >> > >> > Hey Kaxil, >> > I think this is great guidelines. Quick question , do we have tests >> watching an increase in runtime for installing the migration scripts for >> example? Any monitoring there? >> > >> > On Nov 11, 2021, at 4:26 PM, Kaxil Naik <[email protected]> wrote: >> > >> > Hi all committers and reviewers, >> > >> > Let’s be more stricter for PR reviews. Some of the PRs have slipped by >> and merged (I have been guilty too) that had breaking changes in the last >> couple of versions which are now fixed but let's be more vigilant. >> > >> > I propose the following guidelines (not rules): >> > >> > Ask for unit tests coverage wherever applicable >> > Require at least 2 approvals for Core changes >> > Be extra sensitive to DB migrations >> > >> > Verify the logic to confirm that it would not take an unreasonable >> amount of time to run it - especially the ones containing task_instance >> table. >> > Use the utilities added in >> https://github.com/apache/airflow/commit/7622f5e08261afe5ab50a08a6ca0804af8c7c7fe >> to create migrations to avoid cases where for example we miss precisions >> for datetime for MySQL - PR. >> > Ideally, each Migration should be idempotent. >> > >> > At least 1 minor release every 3 months so we don't diverge hugely from >> the main branch >> > >> > >> > Thanks. >> > >> > Regards, >> > Kaxil >> > >> > >> > > > -- > > Leah Cole (she/her) | Developer Programs Engineer, Data Analytics | > [email protected] | +1 (925) 257-2112 > *My working hours may not be your working hours. Please ping me anytime if > you'd like a status update on anything we are working on together - my goal > is to never be a blocker for you. * > >
