Infra: Remove the approval restriction of merging PRs

[zhztheplayer]

Hi folks,

You may notice that in our GitHub repo, at least one approval from committers is needed before a PR is mergeable. This setting is configured at here.

I would like to explore on the possibility of removing this rule. There are a lot of other Apache projects not having this rule but still running perfectly, including the well-known ones like Hadoop, Spark, Arrow, etc. Actually, I found the number of repositories from ASF projects having this restriction is way less than the number of others that don't have this restriction. See my search result:

1. Number of repositories with .asf.yaml defined: ~1100 [1]
2. Number of repositories with .asf.yaml defined with the approval restriction: 228

Hence, I assume in most case a Apache project can run well without this restriction.

Removing the restriction doesn't mean PRs can be arbitrarily merged without essential reviews: It's always the committer's duty to consider carefully on whether review is needed for their PR before merging, One may says people make mistakes, I think that's pretty normal however when mistake happens, we can always revert the unwanted changes to fix. As only committers can merge PRs, I think the risk can be controllable given we always carefully nominate committers. We can observe on the overall cost after the rule change is applied.

As the change can be experimental, I would recommend to remove the rule and observe on the impact for next several weeks, to see if the new rule is suitable for the community.

Do you have any thoughts?

cc @apache/gluten-committers

[1] Note, ASF doesn't have this many of TLPs. The number includes all the sub-repositories under the TLPs (and incubating ones as well).

[jinchengchenghh]

Yes, I agree with that. I would suggest the committer approves the PR first and then left some time for other committers to notice this PR since we don’t need another committee’s approval. Thanks Chengcheng Jin From: Hongze Zhang ***@***.***> Sent: Friday, January 17, 2025 11:17 AM To: apache/incubator-gluten ***@***.***> Cc: Jin, Chengcheng ***@***.***>; Team mention ***@***.***> Subject: [apache/incubator-gluten] Infra: Remove the approval restriction of merging PRs (Discussion #8556) Hi folks, You may notice that in our GitHub repo, at least one approval from committers is needed before a PR is mergeable. This setting is configured at here<https://github.com/apache/incubator-gluten/blob/b09c017dffb4246dc4b29d8cbe6481fa59a50d18/.asf.yaml#L41>. I would like to explore on the possibility of moving this rule. There are a lot of other Apache projects not having this rule but still running perfectly, including the well-known ones like Hadoop, Spark, Arrow, etc. Actually, I found the number of repositories from ASF projects having this restriction is way less than the number of others that don't have this restriction. See my search result: 1. Number of repositories with .asf.yaml defined: ~1100<https://github.com/search?q=org%3Aapache+path%3A%22.asf.yaml%22+%22github%22&type=code&p=5> [1] 2. Number of repositories with .asf.yaml defined with the approval restriction: 228<https://github.com/search?q=org%3Aapache+path%3A%22.asf.yaml%22+%22required_approving_review_count%22&type=code> Hence, I assume in most case a Apache project can run well without this restriction. Removing the restriction doesn't mean PRs can be arbitrarily merged without essential reviews: It's always the committer's duty to consider carefully on whether review is needed for their PR before merging, One may says people make mistakes, I think that's pretty normal however when mistake happens, we can always revert the unwanted changes to fix. As only committers can merge PRs, I think the risk can be controllable given we always carefully nominate committers. We can observe on the overall cost after the rule change is applied. As the change can be experimental, I would recommend to remove the rule and observe on the impact for next several weeks, to see if the new rule is suitable for the community. Do you have any thoughts? cc @apache/gluten-committers<https://github.com/orgs/apache/teams/gluten-committers> [1] Note, ASF doesn't have this much of TLPs. The number includes all the sub-repositories under the TLPs (and incubating ones as well). — Reply to this email directly, view it on GitHub<#8556>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS7NMRV6PYA3WSX2ALQXAIL2LBY3HAVCNFSM6AAAAABVK763HOVHI2DSMVQWIX3LMV43ERDJONRXK43TNFXW4OZXHAZTGNRTGY>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[zhztheplayer]

I would suggest the committer approves the PR first and then left some time for other committers to notice this PR since we don’t need another committee’s approval.

I agree. For minor nit changes, perhaps it's OK to merge with confidence without waiting. But usually one should leave the PR open for some time after being filed to make sure the PR get enough notices from the community.

[JkSelf]

I believe that a committer should still review and approve the PR in Gluten to ensure the robustness. I understand that although the Spark community does not have a strict rule requiring approval before merging code, not everyone who submits code can merge it directly. It usually requires a senior committer to review the code and +1 before it can be merged. Thanks, Jia Ke From: Jin Chengcheng ***@***.***> Sent: Friday, January 17, 2025 1:59 PM To: apache/incubator-gluten ***@***.***> Cc: Jia, Ke A ***@***.***>; Team mention ***@***.***> Subject: Re: [apache/incubator-gluten] Infra: Remove the approval restriction of merging PRs (Discussion #8556) Yes, I agree with that. I would suggest the committer approves the PR first and then left some time for other committers to notice this PR since we don’t need another committee’s approval. Thanks Chengcheng Jin From: Hongze Zhang ***@***.***<mailto:***@***.***>> Sent: Friday, January 17, 2025 11:17 AM To: apache/incubator-gluten ***@***.***<mailto:***@***.***>> Cc: Jin, Chengcheng ***@***.***<mailto:***@***.***>>; Team mention ***@***.***<mailto:***@***.***>> Subject: [apache/incubator-gluten] Infra: Remove the approval restriction of merging PRs (Discussion #8556) Hi folks, You may notice that in our GitHub repo, at least one approval from committers is needed before a PR is mergeable. This setting is configured at here<https://github.com/apache/incubator-gluten/blob/b09c017dffb4246dc4b29d8cbe6481fa59a50d18/.asf.yaml#L41>. I would like to explore on the possibility of moving this rule. There are a lot of other Apache projects not having this rule but still running perfectly, including the well-known ones like Hadoop, Spark, Arrow, etc. Actually, I found the number of repositories from ASF projects having this restriction is way less than the number of others that don't have this restriction. See my search result: 1. Number of repositories with .asf.yaml defined: ~1100<https://github.com/search?q=org%3Aapache+path%3A%22.asf.yaml%22+%22github%22&type=code&p=5> [1] 2. Number of repositories with .asf.yaml defined with the approval restriction: 228<https://github.com/search?q=org%3Aapache+path%3A%22.asf.yaml%22+%22required_approving_review_count%22&type=code> Hence, I assume in most case a Apache project can run well without this restriction. Removing the restriction doesn't mean PRs can be arbitrarily merged without essential reviews: It's always the committer's duty to consider carefully on whether review is needed for their PR before merging, One may says people make mistakes, I think that's pretty normal however when mistake happens, we can always revert the unwanted changes to fix. As only committers can merge PRs, I think the risk can be controllable given we always carefully nominate committers. We can observe on the overall cost after the rule change is applied. As the change can be experimental, I would recommend to remove the rule and observe on the impact for next several weeks, to see if the new rule is suitable for the community. Do you have any thoughts? cc @apache/gluten-committers<https://github.com/orgs/apache/teams/gluten-committers> [1] Note, ASF doesn't have this much of TLPs. The number includes all the sub-repositories under the TLPs (and incubating ones as well). — Reply to this email directly, view it on GitHub<#8556>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS7NMRV6PYA3WSX2ALQXAIL2LBY3HAVCNFSM6AAAAABVK763HOVHI2DSMVQWIX3LMV43ERDJONRXK43TNFXW4OZXHAZTGNRTGY>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***<mailto:***@***.***>> — Reply to this email directly, view it on GitHub<#8556 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC3K7WSSSCWV3OPDFGDKSE32LCLY5AVCNFSM6AAAAABVK763HOVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTCOBWGI3TKOI>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.******@***.***>>

[zhztheplayer]

Thanks and I totally understand your view. Others must have similar concerns.

I understand that although the Spark community does not have a strict rule requiring approval before merging code, not everyone who submits code can merge it directly. It usually requires a senior committer to review the code and +1 before it can be merged.

Spark has enough reviewers for most code modules of its repository. It has plenty of users, contributors, organizations that support the project, as well as formal committers. Because of this, PRs can get reviewed no matter there is a rule to force that or not. If you look at the smaller communities that are not far beyond Gluten's volume, e.g., Zookeeper, Calcite, Tomcat, etc., there are a lot of PRs merged without approvals, and the projects are still maintaining good code / community health.

Moreover, if you look at ASF's committer invitation template, you could find the words "We like to work on trust rather than unnecessary constraints". This is also the meritocracy the ASF praises highly. It doesn't mean a committer can do anything they want, a committer should do best to make the best decisions. If they think their PR needs to be reviewed, then they will send a review request to others. We should trust our committers on their capability to make the good action.

I know the community is used to the currently rule so some may be surprised if we made the change. I'd say give the community some time to see what happens, as we didn't try with the other way since the project entered incubation. If after some time (say, several weeks), we find the new rule doesn't work as expected, we can add the rule back and the whole action should not be harmful.

[zhouyuan]

I was hoping to keep this restriction. Not everyone is as senior as @zhztheplayer and they may make mistakes sometimes. Through code reviews, we can achieve two key objectives:

• Basic Code Quality Control: Ensuring that the code meets our quality standards.
• Onboarding and Knowledge Sharing: Helping team members familiarize themselves with other components (e.g., RAS).
  These are the two critical growth factors currently missing for Gluten IMO. Once we establish a process for thorough and effective reviews, every patch will benefit from meaningful feedback.

I guess your ask is to fast merge on some patches(e.g. big refactors, which may easily get conflict), we could setup a new mechanism: e.g., with a new special label for such PRs

BTW The branch protection(required review) feature is added on 2020 so this maybe one reason why some old projects did not include this:
https://infra.apache.org/blog/even-more-github-features-added.html

Thanks, -yuan

[Yohahaha]

+1

I guess your ask is to fast merge on some patches(e.g. big refactors, which may easily get conflict), we could setup a new mechanism: e.g., with a new special label for such PRs

I dont think refactor need to be fast merged to avoid conflicts, if too many conflicts happens, we should consider whether to break it down into smaller PRs.

[zhztheplayer]

Code conflict is not the key. Code always conflicts, and we don't merge PRs only because of that.

We should consider whether to break it down into smaller PRs.

Actually, if we have an approval restriction, committers will tend to file large PRs than splitting them up. Because the smallest PR still need to wait for approval. Sometimes it's not needed.

[jackylee-ch]

+1 for zhouyuan’s opinion. I think we still need additional committer approvals before code can be merged. In the past, we’ve had several instances where code was reverted after being merged due to objections. If we remove this restriction, I am concerned that more problems will occur.

[zhztheplayer]

@zhouyuan

I guess your ask is to fast merge on some patches(e.g. big refactors, which may easily get conflict)

Fast merging is not the goal. Indeed merges can be faster because of the change, but only for the ones committer is confident with. I am always not confident with my PRs. For example the changes that involve CH code, #8198, #8008, etc. Review is always required from the guys I pinged.

For the objectives mentioned:

Basic Code Quality Control: Ensuring that the code meets our quality standards.

I know this is the purpose of the restriction, but I don't think our code quality is controlled because of the rule. I review a bunch of PRs daily, and I still have to do many follow-up works to maintain the code quality of the code base. The key of avoiding the corruption of code is to encourage all the committers to take more responsibility of useful code reviews, this will be a team work consisting of individual efforts. Simply setting a rule doesn't help any.

Onboarding and Knowledge Sharing: Helping team members familiarize themselves with other components (e.g., RAS).

Code review is a good way for getting more knowledge about the code. But it's not related to the review approvals. One is aways encouraged to learn code through reviewing others' PRs, no matter we have a restriction of approvals or not.

These are the two critical growth factors currently missing for Gluten IMO. Once we establish a process for thorough and effective reviews, every patch will benefit from meaningful feedback.

Again, rules are good, but they don't go with the expected way as you wished. If you set a rule to consider a review with 5 comments is a effective review, then some people may leave unreasoned comments on the smallest PRs which will give you side effect.

The community is all about people. Committers and the PMC members were elected with good reasons, we should trust the people we invited, then build a effective model based on the trust. If we don't have enough experienced committers who we can trust, that will be the community's failure. Adding rules will not cover this kind of failure.

BTW The branch protection(required review) feature is added on 2020.

Git-to-GitHub migration was just done in 2019 in ASF. ASF was founded before 2000. Projects worked pretty well without GitHub in the first 10+ years of ASF. If such rule is required, it should already be documented like "+1 votes are needed for code change" in ASF's documentation before 2020. As I said, the ASF encourages meritocracy. This is the reason why there is not such restrictions from the ASF.

[zhztheplayer]

+1 for zhouyuan’s opinion. I think we still need additional committer approvals before code can be merged. In the past, we’ve had several instances where code was reverted after being merged due to objections. If we remove this restriction, I am concerned that more problems will occur.

I understand, so I recommend to try with no restrictions for several weeks to see if we have more cost because of the change. If we do see many PRs were reverted because of no reviews, we could add the rule back. Does that sound work for you? It will be worth it if we find the change doesn't bring burden (e.g., many code reverts) to the community.

[FelixYBW]

Let's vote directly among committers. Several weeks can't make any difference

[jinchengchenghh]

+1 for zhouyuan’s opinion. And we should let the committers be familiar with other committee's work, split the project to several modules, each module has more than 1 owner, so the features can be carefully reviewed and may feed back constructive comments, then other committee's approval is essential.

[zhli1142015]

+1 for zhouyuan’s opinion and listing owners for modules.

[liujiayi771]

+1 for zhouyuan’s opinion, we should be more cautious for merging PRs.

[zhanglistar]

+1 for zhouyuan’s opinion, we should be more cautious for merging PRs.

[yma11]

+1 for zhouyuan’s opinion. I think Gluten community is not so mature as Spark and doesn’t have such confidence to maintain the repo with such a relaxed rule. From: zhanglistar ***@***.***> Sent: Saturday, January 18, 2025 4:21 PM To: apache/incubator-gluten ***@***.***> Cc: Ma, Yan ***@***.***>; Team mention ***@***.***> Subject: Re: [apache/incubator-gluten] Infra: Remove the approval restriction of merging PRs (Discussion #8556) +1 for zhouyuan’s opinion, we should be more cautious for merging PRs. — Reply to this email directly, view it on GitHub<#8556 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC2M2YHIJQNMHRKEGTC2ATL2LIFG7AVCNFSM6AAAAABVK763HOVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTCOBXGM4TKMI>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.******@***.***>>

[baibaichen]

+1 for yuan's option.

[lgbo-ustc]

+1 for yuan's option Chang chen ***@***.***> 于2025年1月19日周日 13:13写道：

…

+1 for yuan's option. — Reply to this email directly, view it on GitHub <#8556 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAT2UF5GKMIQHT2VA3TAOH32LMX73AVCNFSM6AAAAABVK763HOVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTCOBXHEZTSNQ> . You are receiving this because you are on a team that was mentioned.Message ID: <apache/incubator-gluten/repo-discussions/8556/comments/11879396@ github.com>

-- from 梁家彪

[taiyang-li]

+1 for zhouyuan’s opinion

[kecookier]

+1 for keeping the approval restriction.

[loneylee]

+1 for zhouyuan’s opinion

[zhztheplayer]

Closing due to the objections. Thank you for sharing your views.