You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dubbo.apache.org by Huxing Zhang <hu...@apache.org> on 2019/03/07 09:04:03 UTC
Pull request review rules
Hi,
When I am looking at the pull request, I found a pull request[1] got
approved by 2 of our reviewers(committers), but still not getting
merged.
I am thinking why it is like this. Should we set up community rules
for thing like this?
For example, if a pull request has got at least N approval from
committers, it can be merged, where N can be discussed. The more
approval it need, the longer process it will take.
For large size pull requests, the reviewer can request another one to
help on it.
I would suggest to keep it small, N=1. Even the reviewer fails to
identify the issues, it can be fixed by sending another pull request.
How do you think?
[1] https://github.com/apache/incubator-dubbo/pull/3536
--
Best Regards！
Huxing
Re: Pull request review rules
Posted by Huxing Zhang <hu...@apache.org>.
Hi,
I also suggest the reviewer to follow the standard reviewing
process[1], rather than just leave a comment.
To me, the advantage are as follows:
- review status will be displayed in pull request, one can easily
identify pull request that is in "request change" so that you won't
look into it again.
- all the review stats/activities will be collected by dubbo-bot and
summarized in the weekly report.
[1] https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
On Thu, Mar 7, 2019 at 5:04 PM Huxing Zhang <hu...@apache.org> wrote:
>
> Hi,
>
> When I am looking at the pull request, I found a pull request[1] got
> approved by 2 of our reviewers(committers), but still not getting
> merged.
>
> I am thinking why it is like this. Should we set up community rules
> for thing like this?
> For example, if a pull request has got at least N approval from
> committers, it can be merged, where N can be discussed. The more
> approval it need, the longer process it will take.
>
> For large size pull requests, the reviewer can request another one to
> help on it.
>
> I would suggest to keep it small, N=1. Even the reviewer fails to
> identify the issues, it can be fixed by sending another pull request.
>
> How do you think?
>
> [1] https://github.com/apache/incubator-dubbo/pull/3536
>
> --
> Best Regards！
> Huxing
--
Best Regards！
Huxing
Re: Pull request review rules
Posted by Ian Luo <ia...@gmail.com>.
Agree, a deadline is a good suggestion. It requires all committers look
into pull request more actively.
-Ian.
On Fri, Mar 8, 2019 at 10:47 AM yuhang xiu <ca...@gmail.com> wrote:
> Hi, all
>
> In my opinion, both suggestions are good.
> But what I need to remind is that we should have at least one deadline to
> prevent a pr from being suspended for a long time.
>
> Thx.
>
> Ian Luo <ia...@gmail.com> 于2019年3月8日周五 上午10:41写道：
>
> > I think we don't need such a complex rule at all. More reviewers on it,
> > more confidence it will give the author. I think there's a chance for
> > reviewers more than one looking into one pull request because this pull
> > request may look interesting to them. I don't worry too much since
> > eventually they will reach a consensus and the pull request get merged by
> > one of the reviewers. What I suggest is to make the rule simple, no pull
> > request can be merged unless it's reviewed one reviewer at least.
> >
> > Thanks,
> > -Ian.
> >
> >
> > On Fri, Mar 8, 2019 at 10:37 AM Ian Luo <ia...@gmail.com> wrote:
> >
> > > I think we don't need such a complex rule at all. More reviewers on it,
> > > more confidence it will give the author. I think there's a chance for
> > > reviewers more than one looking into one pull request because this pull
> > > request may look interesting
> > >
> > > On Thu, Mar 7, 2019 at 5:04 PM Huxing Zhang <hu...@apache.org> wrote:
> > >
> > >> Hi,
> > >>
> > >> When I am looking at the pull request, I found a pull request[1] got
> > >> approved by 2 of our reviewers(committers), but still not getting
> > >> merged.
> > >>
> > >> I am thinking why it is like this. Should we set up community rules
> > >> for thing like this?
> > >> For example, if a pull request has got at least N approval from
> > >> committers, it can be merged, where N can be discussed. The more
> > >> approval it need, the longer process it will take.
> > >>
> > >> For large size pull requests, the reviewer can request another one to
> > >> help on it.
> > >>
> > >> I would suggest to keep it small, N=1. Even the reviewer fails to
> > >> identify the issues, it can be fixed by sending another pull request.
> > >>
> > >> How do you think?
> > >>
> > >> [1] https://github.com/apache/incubator-dubbo/pull/3536
> > >>
> > >> --
> > >> Best Regards！
> > >> Huxing
> > >>
> > >
> >
>
Re: Pull request review rules
Posted by yuhang xiu <ca...@gmail.com>.
Hi, all
In my opinion, both suggestions are good.
But what I need to remind is that we should have at least one deadline to
prevent a pr from being suspended for a long time.
Thx.
Ian Luo <ia...@gmail.com> 于2019年3月8日周五 上午10:41写道：
> I think we don't need such a complex rule at all. More reviewers on it,
> more confidence it will give the author. I think there's a chance for
> reviewers more than one looking into one pull request because this pull
> request may look interesting to them. I don't worry too much since
> eventually they will reach a consensus and the pull request get merged by
> one of the reviewers. What I suggest is to make the rule simple, no pull
> request can be merged unless it's reviewed one reviewer at least.
>
> Thanks,
> -Ian.
>
>
> On Fri, Mar 8, 2019 at 10:37 AM Ian Luo <ia...@gmail.com> wrote:
>
> > I think we don't need such a complex rule at all. More reviewers on it,
> > more confidence it will give the author. I think there's a chance for
> > reviewers more than one looking into one pull request because this pull
> > request may look interesting
> >
> > On Thu, Mar 7, 2019 at 5:04 PM Huxing Zhang <hu...@apache.org> wrote:
> >
> >> Hi,
> >>
> >> When I am looking at the pull request, I found a pull request[1] got
> >> approved by 2 of our reviewers(committers), but still not getting
> >> merged.
> >>
> >> I am thinking why it is like this. Should we set up community rules
> >> for thing like this?
> >> For example, if a pull request has got at least N approval from
> >> committers, it can be merged, where N can be discussed. The more
> >> approval it need, the longer process it will take.
> >>
> >> For large size pull requests, the reviewer can request another one to
> >> help on it.
> >>
> >> I would suggest to keep it small, N=1. Even the reviewer fails to
> >> identify the issues, it can be fixed by sending another pull request.
> >>
> >> How do you think?
> >>
> >> [1] https://github.com/apache/incubator-dubbo/pull/3536
> >>
> >> --
> >> Best Regards！
> >> Huxing
> >>
> >
>
Re: Pull request review rules
Posted by Ian Luo <ia...@gmail.com>.
I think we don't need such a complex rule at all. More reviewers on it,
more confidence it will give the author. I think there's a chance for
reviewers more than one looking into one pull request because this pull
request may look interesting to them. I don't worry too much since
eventually they will reach a consensus and the pull request get merged by
one of the reviewers. What I suggest is to make the rule simple, no pull
request can be merged unless it's reviewed one reviewer at least.
Thanks,
-Ian.
On Fri, Mar 8, 2019 at 10:37 AM Ian Luo <ia...@gmail.com> wrote:
> I think we don't need such a complex rule at all. More reviewers on it,
> more confidence it will give the author. I think there's a chance for
> reviewers more than one looking into one pull request because this pull
> request may look interesting
>
> On Thu, Mar 7, 2019 at 5:04 PM Huxing Zhang <hu...@apache.org> wrote:
>
>> Hi,
>>
>> When I am looking at the pull request, I found a pull request[1] got
>> approved by 2 of our reviewers(committers), but still not getting
>> merged.
>>
>> I am thinking why it is like this. Should we set up community rules
>> for thing like this?
>> For example, if a pull request has got at least N approval from
>> committers, it can be merged, where N can be discussed. The more
>> approval it need, the longer process it will take.
>>
>> For large size pull requests, the reviewer can request another one to
>> help on it.
>>
>> I would suggest to keep it small, N=1. Even the reviewer fails to
>> identify the issues, it can be fixed by sending another pull request.
>>
>> How do you think?
>>
>> [1] https://github.com/apache/incubator-dubbo/pull/3536
>>
>> --
>> Best Regards！
>> Huxing
>>
>
Re: Pull request review rules
Posted by Ian Luo <ia...@gmail.com>.
I think we don't need such a complex rule at all. More reviewers on it,
more confidence it will give the author. I think there's a chance for
reviewers more than one looking into one pull request because this pull
request may look interesting
On Thu, Mar 7, 2019 at 5:04 PM Huxing Zhang <hu...@apache.org> wrote:
> Hi,
>
> When I am looking at the pull request, I found a pull request[1] got
> approved by 2 of our reviewers(committers), but still not getting
> merged.
>
> I am thinking why it is like this. Should we set up community rules
> for thing like this?
> For example, if a pull request has got at least N approval from
> committers, it can be merged, where N can be discussed. The more
> approval it need, the longer process it will take.
>
> For large size pull requests, the reviewer can request another one to
> help on it.
>
> I would suggest to keep it small, N=1. Even the reviewer fails to
> identify the issues, it can be fixed by sending another pull request.
>
> How do you think?
>
> [1] https://github.com/apache/incubator-dubbo/pull/3536
>
> --
> Best Regards！
> Huxing
>
Re: Pull request review rules
Posted by Huxing Zhang <hu...@apache.org>.
Hi,
On Thu, Mar 7, 2019 at 5:42 PM LiZhenNet <li...@gmail.com> wrote:
>
> Hi，Huxing
> Your suggestion is right, I mean we should pay attention to the previously
> submitted issue.
> Close? Merge? or Add comment .
> We should resolve the issue that was created for a long time as soon as
> possible.
+1 but this is beyond the scope of this thread, I think we can discuss
it in another thread.
>
>
> Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:28写道：
>
> > Hi,
> >
> > On Thu, Mar 7, 2019 at 5:12 PM LiZhenNet <li...@gmail.com> wrote:
> > >
> > > Hi，
> > > I don’t think it’s a problem about number of reviewers , As time goes by,
> > > we will slowly ignore it,We pay more attention to the new issue.
> >
> > Are you suggesting not to set up such a hard limit?
> > It just looks strange to me, that 2 of the reviewer (with write
> > access) has approve the pull request, but it is not getting merged.
> > So what does the 2 approval mean?
> >
> > >
> > >
> > >
> > > Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
> > >
> > > > Hi,
> > > >
> > > > When I am looking at the pull request, I found a pull request[1] got
> > > > approved by 2 of our reviewers(committers), but still not getting
> > > > merged.
> > > >
> > > > I am thinking why it is like this. Should we set up community rules
> > > > for thing like this?
> > > > For example, if a pull request has got at least N approval from
> > > > committers, it can be merged, where N can be discussed. The more
> > > > approval it need, the longer process it will take.
> > > >
> > > > For large size pull requests, the reviewer can request another one to
> > > > help on it.
> > > >
> > > > I would suggest to keep it small, N=1. Even the reviewer fails to
> > > > identify the issues, it can be fixed by sending another pull request.
> > > >
> > > > How do you think?
> > > >
> > > > [1] https://github.com/apache/incubator-dubbo/pull/3536
> > > >
> > > > --
> > > > Best Regards！
> > > > Huxing
> > > >
> >
> >
> >
> > --
> > Best Regards！
> > Huxing
> >
--
Best Regards！
Huxing
Re: Pull request review rules
Posted by LiZhenNet <li...@gmail.com>.
Hi，Huxing
Your suggestion is right, I mean we should pay attention to the previously
submitted issue.
Close? Merge? or Add comment .
We should resolve the issue that was created for a long time as soon as
possible.
Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:28写道：
> Hi,
>
> On Thu, Mar 7, 2019 at 5:12 PM LiZhenNet <li...@gmail.com> wrote:
> >
> > Hi，
> > I don’t think it’s a problem about number of reviewers , As time goes by,
> > we will slowly ignore it,We pay more attention to the new issue.
>
> Are you suggesting not to set up such a hard limit?
> It just looks strange to me, that 2 of the reviewer (with write
> access) has approve the pull request, but it is not getting merged.
> So what does the 2 approval mean?
>
> >
> >
> >
> > Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
> >
> > > Hi,
> > >
> > > When I am looking at the pull request, I found a pull request[1] got
> > > approved by 2 of our reviewers(committers), but still not getting
> > > merged.
> > >
> > > I am thinking why it is like this. Should we set up community rules
> > > for thing like this?
> > > For example, if a pull request has got at least N approval from
> > > committers, it can be merged, where N can be discussed. The more
> > > approval it need, the longer process it will take.
> > >
> > > For large size pull requests, the reviewer can request another one to
> > > help on it.
> > >
> > > I would suggest to keep it small, N=1. Even the reviewer fails to
> > > identify the issues, it can be fixed by sending another pull request.
> > >
> > > How do you think?
> > >
> > > [1] https://github.com/apache/incubator-dubbo/pull/3536
> > >
> > > --
> > > Best Regards！
> > > Huxing
> > >
>
>
>
> --
> Best Regards！
> Huxing
>
Re: Pull request review rules
Posted by jun liu <ke...@gmail.com>.
For this particular PR, the reason I approved but not merged it is that I think the code change itself is ok but I don’t understand the connection with the issue the author claims to solve. I plan to merge it after fully understand, unfortunately, I just forgot to check the feedback.
Jun
> On Mar 7, 2019, at 5:27 PM, Huxing Zhang <hu...@apache.org> wrote:
>
> Hi,
>
> On Thu, Mar 7, 2019 at 5:12 PM LiZhenNet <lizhencoder@gmail.com <ma...@gmail.com>> wrote:
>>
>> Hi，
>> I don’t think it’s a problem about number of reviewers , As time goes by,
>> we will slowly ignore it,We pay more attention to the new issue.
>
> Are you suggesting not to set up such a hard limit?
> It just looks strange to me, that 2 of the reviewer (with write
> access) has approve the pull request, but it is not getting merged.
> So what does the 2 approval mean?
>
>>
>>
>>
>> Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
>>
>>> Hi,
>>>
>>> When I am looking at the pull request, I found a pull request[1] got
>>> approved by 2 of our reviewers(committers), but still not getting
>>> merged.
>>>
>>> I am thinking why it is like this. Should we set up community rules
>>> for thing like this?
>>> For example, if a pull request has got at least N approval from
>>> committers, it can be merged, where N can be discussed. The more
>>> approval it need, the longer process it will take.
>>>
>>> For large size pull requests, the reviewer can request another one to
>>> help on it.
>>>
>>> I would suggest to keep it small, N=1. Even the reviewer fails to
>>> identify the issues, it can be fixed by sending another pull request.
>>>
>>> How do you think?
>>>
>>> [1] https://github.com/apache/incubator-dubbo/pull/3536
>>>
>>> --
>>> Best Regards！
>>> Huxing
>>>
>
>
>
> --
> Best Regards！
> Huxing
Re: Pull request review rules
Posted by Huxing Zhang <hu...@apache.org>.
Hi,
On Thu, Mar 7, 2019 at 5:12 PM LiZhenNet <li...@gmail.com> wrote:
>
> Hi，
> I don’t think it’s a problem about number of reviewers , As time goes by,
> we will slowly ignore it,We pay more attention to the new issue.
Are you suggesting not to set up such a hard limit?
It just looks strange to me, that 2 of the reviewer (with write
access) has approve the pull request, but it is not getting merged.
So what does the 2 approval mean?
>
>
>
> Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
>
> > Hi,
> >
> > When I am looking at the pull request, I found a pull request[1] got
> > approved by 2 of our reviewers(committers), but still not getting
> > merged.
> >
> > I am thinking why it is like this. Should we set up community rules
> > for thing like this?
> > For example, if a pull request has got at least N approval from
> > committers, it can be merged, where N can be discussed. The more
> > approval it need, the longer process it will take.
> >
> > For large size pull requests, the reviewer can request another one to
> > help on it.
> >
> > I would suggest to keep it small, N=1. Even the reviewer fails to
> > identify the issues, it can be fixed by sending another pull request.
> >
> > How do you think?
> >
> > [1] https://github.com/apache/incubator-dubbo/pull/3536
> >
> > --
> > Best Regards！
> > Huxing
> >
--
Best Regards！
Huxing
Re: Pull request review rules
Posted by 华 钟明 <cr...@gmail.com>.
I think it is reasonable to set up one reviewer.
Because reviewing a pr itself requires a reviewer, and the reviewer can also self-mark, indicating that he wants to keep an eye on the pr.
Reviewers prefer to choose from the issue's discussants.
Best Regards！
Zhongming Hua
在 2019/3/7 下午5:12，“LiZhenNet”<li...@gmail.com> 写入:
Hi，
I don’t think it’s a problem about number of reviewers , As time goes by,
we will slowly ignore it,We pay more attention to the new issue.
Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
> Hi,
>
> When I am looking at the pull request, I found a pull request[1] got
> approved by 2 of our reviewers(committers), but still not getting
> merged.
>
> I am thinking why it is like this. Should we set up community rules
> for thing like this?
> For example, if a pull request has got at least N approval from
> committers, it can be merged, where N can be discussed. The more
> approval it need, the longer process it will take.
>
> For large size pull requests, the reviewer can request another one to
> help on it.
>
> I would suggest to keep it small, N=1. Even the reviewer fails to
> identify the issues, it can be fixed by sending another pull request.
>
> How do you think?
>
> [1] https://github.com/apache/incubator-dubbo/pull/3536
>
> --
> Best Regards！
> Huxing
>
Re: Pull request review rules
Posted by LiZhenNet <li...@gmail.com>.
Hi，
I don’t think it’s a problem about number of reviewers , As time goes by,
we will slowly ignore it,We pay more attention to the new issue.
Huxing Zhang <hu...@apache.org> 于2019年3月7日周四 下午5:04写道：
> Hi,
>
> When I am looking at the pull request, I found a pull request[1] got
> approved by 2 of our reviewers(committers), but still not getting
> merged.
>
> I am thinking why it is like this. Should we set up community rules
> for thing like this?
> For example, if a pull request has got at least N approval from
> committers, it can be merged, where N can be discussed. The more
> approval it need, the longer process it will take.
>
> For large size pull requests, the reviewer can request another one to
> help on it.
>
> I would suggest to keep it small, N=1. Even the reviewer fails to
> identify the issues, it can be fixed by sending another pull request.
>
> How do you think?
>
> [1] https://github.com/apache/incubator-dubbo/pull/3536
>
> --
> Best Regards！
> Huxing
>