Re: Committers: Let's be more strict for PR Reviews

[James Timmins <ja...@astronomer.io.INVALID>]

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 <ja...@potiuk.com>, 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
> <ah...@pinterest.com.invalid> 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 <ka...@apache.org> 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
> >
> >

Re: Committers: Let's be more strict for PR Reviews

[Kaxil Naik <ka...@gmail.com>]

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 <ja...@astronomer.io.invalid>
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 <ja...@potiuk.com>, 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
> <ah...@pinterest.com.invalid> 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 <ka...@apache.org> 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
>
>
>