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 Nov 11, 2021, 11:03 PM -0800, 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 > > >
