[Governance] Should we leave PRs open for some minimum time to allow for reviews?

[andygrove]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
DataFusion is a very active fast-moving project and I am starting to see some issues with the pace. We often have PRs being merged within 12 hours of being filed ( know I am guilty of doing this) and this means that contributors in other timezones do not have the opportunity to review. I know that I have missed out on reviewing PRs that impact my projects because I was busy on the day that the PR got created and merged and it can be frustrating. I am seeing this happen to other contributors as well.

I'd like to propose that we start leaving major PRs open for at least 24 hours. This is at least enough time for contributors to comment if they need more time to review. I am not sure what the definition of a "major PR" is, but I think it should include PRs that have breaking changes (such as API changes or optimizer changes that cause plans to be re-written differently, which is effectively an API change as well).

We should write something up in the contributing guide explaining whatever policy we decide on.

Describe the solution you'd like
See above.

Describe alternatives you've considered
None

Additional context
None

[andygrove]

@alamb @tustvold @Dandandan Let me know what you think

[avantgardnerio]

+1 FWIW - I think it will be a little painful sometimes, but is probably the lightest weight change we could make to policy to not have the opposite issue of having work merged, but then worrying about it being reverted again.

[xudong963]

+1.

For some minor or chore changes, I think it doesn't matter. But for big changes (such as API change, architect change) or some controversial changes, we need to give it more time (more than 24 hours) to be reviewed by at least two reviewers.

[comphead]

@andygrove imho increasing min required reviewers to 2 might help also

[Dandandan]

+1
I would also advocate for trying to keep this a "common sense" policy: only when there is a substantial change in the API or design.

So, some examples of what I think we should be able to merge within 24 hours:

• Documentation improvements/additions
• Most bug fixes
• Non-controversial build-related changes (clippy, version upgrades etc.)
• Smaller non-controversial feature additions

[alamb]

I'd like to propose that we start leaving major PRs open for at least 24 hours.

I agree 100% with this. Making it common sense so we can merge small changes is also fine with me 👍 Anything marked as API change is for sure should be left open for 24 hours.

@andygrove imho increasing min required reviewers to 2 might help also

I would be in favor of this change only if more people in the community can step up to review PRs (especially larger or trickier changes). I already see a struggle to get such PRs reviewed in a timely manner. Adding additional review burden when I think it is one of our most limited resources already does not seem like a good idea to me

Not only have we have added several additional committers over the last 12 months, I found reviews by anyone are super helpful as they can identify issues (like lack of tests, other features, etc) that can be addressed and make future reviews faster.

[alamb]

One minor variation is to wait for 24 hours after a major PR is approved to merge it. That way people can see what PRs are candidates to be merged and can prioritize their review more appropriately.

[avantgardnerio]

Adding additional review burden when I think it is one of our most limited resources already does not seem like a good idea to me

💯 contingent upon resources we don't yet have (and my apologies for not being more active myself).

[yahoNanJing]

+1

Totally agree for leaving PRs with API changes open for 24 hours.

[alamb]

working on a PR with some proposed wording here

[alamb]

Proposal in #3985