You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dubbo.apache.org by Ian Luo <ia...@gmail.com> on 2019/03/14 14:02:54 UTC
need to revisit pull request 3549 in 2.7.2
Folks,
I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
fluent api need to be carefully resigned.
What do you think?
Thanks,
-Ian.
Re: need to revisit pull request 3549 in 2.7.2
Posted by yuhang xiu <ca...@gmail.com>.
Hi,
+1
This pr is very big. I also left some comments under it. This feature
should be considered more clearly.
jun liu <ke...@gmail.com> 于2019年3月15日周五 上午10:54写道：
> Hi,
>
> +1
>
> I think we should revert this merge and postpone to 2.7.2, it has not been
> fully discussed yet.
>
> Jun
>
> > On Mar 14, 2019, at 10:02 PM, Ian Luo <ia...@gmail.com> wrote:
> >
> > Folks,
> >
> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> > fluent api need to be carefully resigned.
> >
> > What do you think?
> >
> > Thanks,
> > -Ian.
>
>
Re: need to revisit pull request 3549 in 2.7.2
Posted by jun liu <ke...@gmail.com>.
Hi,
+1
I think we should revert this merge and postpone to 2.7.2, it has not been fully discussed yet.
Jun
> On Mar 14, 2019, at 10:02 PM, Ian Luo <ia...@gmail.com> wrote:
>
> Folks,
>
> I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> fluent api need to be carefully resigned.
>
> What do you think?
>
> Thanks,
> -Ian.
Re: need to revisit pull request 3549 in 2.7.2
Posted by Huxing Zhang <hu...@gmail.com>.
Hi,
On Fri, Mar 15, 2019 at 4:28 PM yuhang xiu <ca...@gmail.com> wrote:
>
> Hi, huxing.
>
> Great advice. In the previous review process, I also ignored some issues.
>
> Maybe we need a new label such as: `need-more-review`. When we need more
> reviews, we can mark this pr to remind other committers to review it.
+1 to a label called NEED-MORE-REVIEW. Good idea!
>
> In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I
> think there is only one case where we will use this label, which is the
> request that is approved, but this pr has not passed ci.
+1
>
> In this pr merger, there are some problems, and we need to pay more
> attention in the future.
>
> Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
>
> > Hi,
> >
> > Looking at the reviewing process of the pull request, I can see some
> > issues here:
> > - No one has officially approved the pull request before it getting
> > merged. By saying officially I mean this[2], which I've mentioned in
> > another thread.
> > - One of the reviewer has marked the milestone as 2.7.2, but applied
> > it with a label called STATUS/READY-TO-MERGE. These two activities
> > look contradictory but according to the context he's meaning I guess
> > is to put some more time to it.
> > - The one who merged this pull request, just merge this pull request
> > without saying anything. I am not sure he mis-understand the
> > STATUS/READY-TO-MERGE label or not. But I think if read the context
> > through, one should be more cautious to merge that pull request.
> >
> > What I think can improve it:
> > - Every pull request must be approved officially by at least one of
> > the committer. (Comments such as LGTM does not mean official
> > approval.)
> > - If you feel uncertain about a pull request, please comment (see step
> > 6 in [2]) that you need more review to come in to review or request
> > review from others directly
> > - If you want to merge the pull request, you must read through all the
> > context.
> >
> > How do you think?
> >
> > [1] https://github.com/apache/incubator-dubbo/pull/3549
> > [2]
> > https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
> >
> > On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
> > >
> > > Folks,
> > >
> > > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> > > fluent api need to be carefully resigned.
> > >
> > > What do you think?
> > >
> > > Thanks,
> > > -Ian.
> >
> >
> >
> > --
> > Best Regards！
> > Huxing
> >
--
Best Regards！
Huxing
Re: Re: need to revisit pull request 3549 in 2.7.2
Posted by yuhang xiu <ca...@gmail.com>.
Sorry about that..
I mean `hi xu`. Wrong name of this statement..
yuhang xiu <ca...@gmail.com> 于2019年3月15日周五 下午4:33写道：
> Hi jun,
>
> Not a problem.
>
> It’s just that we need to pay more attention in the future.
>
> :)
>
> kezhenxu94 <ke...@163.com> 于2019年3月15日周五 下午4:31写道：
>
>> Hi,
>>
>>
>> > - The one who merged this pull request, just merge this pull request
>> > without saying anything. I am not sure he mis-understand the
>> > STATUS/READY-TO-MERGE label or not. But I think if read the context
>> > through, one should be more cautious to merge that pull request.
>>
>>
>> I'm so sorry it was me who merged that pull request.
>> I had read through the context(actually I reviewed although I did not
>> comment)
>> and indeed I've misunderstood the STATUS/READY-TO-MERGE label,
>> and I suggest put more detail about the labels in the committer guide[1]
>> to avoid later committers making the same mistakes.
>>
>>
>> Sorry again for making such mistake, I'll revert it if needed.
>>
>>
>>
>>
>> 1.
>> http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> At 2019-03-15 16:28:16, "yuhang xiu" <ca...@gmail.com> wrote:
>> >Hi, huxing.
>> >
>> >Great advice. In the previous review process, I also ignored some issues.
>> >
>> >Maybe we need a new label such as: `need-more-review`. When we need more
>> >reviews, we can mark this pr to remind other committers to review it.
>> >
>> >In addition, I think we need to clarify the meaning of `READY-TO-MERGE`.
>> I
>> >think there is only one case where we will use this label, which is the
>> >request that is approved, but this pr has not passed ci.
>> >
>> >In this pr merger, there are some problems, and we need to pay more
>> >attention in the future.
>> >
>> >Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
>> >
>> >> Hi,
>> >>
>> >> Looking at the reviewing process of the pull request, I can see some
>> >> issues here:
>> >> - No one has officially approved the pull request before it getting
>> >> merged. By saying officially I mean this[2], which I've mentioned in
>> >> another thread.
>> >> - One of the reviewer has marked the milestone as 2.7.2, but applied
>> >> it with a label called STATUS/READY-TO-MERGE. These two activities
>> >> look contradictory but according to the context he's meaning I guess
>> >> is to put some more time to it.
>> >> - The one who merged this pull request, just merge this pull request
>> >> without saying anything. I am not sure he mis-understand the
>> >> STATUS/READY-TO-MERGE label or not. But I think if read the context
>> >> through, one should be more cautious to merge that pull request.
>> >>
>> >> What I think can improve it:
>> >> - Every pull request must be approved officially by at least one of
>> >> the committer. (Comments such as LGTM does not mean official
>> >> approval.)
>> >> - If you feel uncertain about a pull request, please comment (see step
>> >> 6 in [2]) that you need more review to come in to review or request
>> >> review from others directly
>> >> - If you want to merge the pull request, you must read through all the
>> >> context.
>> >>
>> >> How do you think?
>> >>
>> >> [1] https://github.com/apache/incubator-dubbo/pull/3549
>> >> [2]
>> >>
>> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
>> >>
>> >> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
>> >> >
>> >> > Folks,
>> >> >
>> >> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
>> >> > fluent api need to be carefully resigned.
>> >> >
>> >> > What do you think?
>> >> >
>> >> > Thanks,
>> >> > -Ian.
>> >>
>> >>
>> >>
>> >> --
>> >> Best Regards！
>> >> Huxing
>> >>
>>
>
Re: Re: need to revisit pull request 3549 in 2.7.2
Posted by yuhang xiu <ca...@gmail.com>.
Hi jun,
Not a problem.
It’s just that we need to pay more attention in the future.
:)
kezhenxu94 <ke...@163.com> 于2019年3月15日周五 下午4:31写道：
> Hi,
>
>
> > - The one who merged this pull request, just merge this pull request
> > without saying anything. I am not sure he mis-understand the
> > STATUS/READY-TO-MERGE label or not. But I think if read the context
> > through, one should be more cautious to merge that pull request.
>
>
> I'm so sorry it was me who merged that pull request.
> I had read through the context(actually I reviewed although I did not
> comment)
> and indeed I've misunderstood the STATUS/READY-TO-MERGE label,
> and I suggest put more detail about the labels in the committer guide[1]
> to avoid later committers making the same mistakes.
>
>
> Sorry again for making such mistake, I'll revert it if needed.
>
>
>
>
> 1.
> http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html
>
>
>
>
>
>
>
>
>
>
> At 2019-03-15 16:28:16, "yuhang xiu" <ca...@gmail.com> wrote:
> >Hi, huxing.
> >
> >Great advice. In the previous review process, I also ignored some issues.
> >
> >Maybe we need a new label such as: `need-more-review`. When we need more
> >reviews, we can mark this pr to remind other committers to review it.
> >
> >In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I
> >think there is only one case where we will use this label, which is the
> >request that is approved, but this pr has not passed ci.
> >
> >In this pr merger, there are some problems, and we need to pay more
> >attention in the future.
> >
> >Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
> >
> >> Hi,
> >>
> >> Looking at the reviewing process of the pull request, I can see some
> >> issues here:
> >> - No one has officially approved the pull request before it getting
> >> merged. By saying officially I mean this[2], which I've mentioned in
> >> another thread.
> >> - One of the reviewer has marked the milestone as 2.7.2, but applied
> >> it with a label called STATUS/READY-TO-MERGE. These two activities
> >> look contradictory but according to the context he's meaning I guess
> >> is to put some more time to it.
> >> - The one who merged this pull request, just merge this pull request
> >> without saying anything. I am not sure he mis-understand the
> >> STATUS/READY-TO-MERGE label or not. But I think if read the context
> >> through, one should be more cautious to merge that pull request.
> >>
> >> What I think can improve it:
> >> - Every pull request must be approved officially by at least one of
> >> the committer. (Comments such as LGTM does not mean official
> >> approval.)
> >> - If you feel uncertain about a pull request, please comment (see step
> >> 6 in [2]) that you need more review to come in to review or request
> >> review from others directly
> >> - If you want to merge the pull request, you must read through all the
> >> context.
> >>
> >> How do you think?
> >>
> >> [1] https://github.com/apache/incubator-dubbo/pull/3549
> >> [2]
> >>
> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
> >>
> >> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
> >> >
> >> > Folks,
> >> >
> >> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> >> > fluent api need to be carefully resigned.
> >> >
> >> > What do you think?
> >> >
> >> > Thanks,
> >> > -Ian.
> >>
> >>
> >>
> >> --
> >> Best Regards！
> >> Huxing
> >>
>
Re: Re: need to revisit pull request 3549 in 2.7.2
Posted by Ian Luo <ia...@gmail.com>.
We'd better set up an official protocol for pull request that the whole
community will honor and follow.
Thanks,
-Ian.
On Fri, Mar 15, 2019 at 5:18 PM Huxing Zhang <hu...@apache.org> wrote:
> On Fri, Mar 15, 2019 at 4:31 PM kezhenxu94 <ke...@163.com> wrote:
> >
> > Hi,
> >
> >
> > > - The one who merged this pull request, just merge this pull request
> > > without saying anything. I am not sure he mis-understand the
> > > STATUS/READY-TO-MERGE label or not. But I think if read the context
> > > through, one should be more cautious to merge that pull request.
> >
> >
> > I'm so sorry it was me who merged that pull request.
> > I had read through the context(actually I reviewed although I did not
> comment)
> > and indeed I've misunderstood the STATUS/READY-TO-MERGE label,
> > and I suggest put more detail about the labels in the committer guide[1]
> > to avoid later committers making the same mistakes.
>
> Big +1 to that.
>
> >
> >
> > Sorry again for making such mistake, I'll revert it if needed.
>
> I think there is no need to revert unless there is some serious issues
> (not looking very deeply though), one can improve it by sending
> another pull request if possible.
>
> >
> >
> >
> >
> > 1.
> http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > At 2019-03-15 16:28:16, "yuhang xiu" <ca...@gmail.com> wrote:
> > >Hi, huxing.
> > >
> > >Great advice. In the previous review process, I also ignored some
> issues.
> > >
> > >Maybe we need a new label such as: `need-more-review`. When we need more
> > >reviews, we can mark this pr to remind other committers to review it.
> > >
> > >In addition, I think we need to clarify the meaning of
> `READY-TO-MERGE`. I
> > >think there is only one case where we will use this label, which is the
> > >request that is approved, but this pr has not passed ci.
> > >
> > >In this pr merger, there are some problems, and we need to pay more
> > >attention in the future.
> > >
> > >Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
> > >
> > >> Hi,
> > >>
> > >> Looking at the reviewing process of the pull request, I can see some
> > >> issues here:
> > >> - No one has officially approved the pull request before it getting
> > >> merged. By saying officially I mean this[2], which I've mentioned in
> > >> another thread.
> > >> - One of the reviewer has marked the milestone as 2.7.2, but applied
> > >> it with a label called STATUS/READY-TO-MERGE. These two activities
> > >> look contradictory but according to the context he's meaning I guess
> > >> is to put some more time to it.
> > >> - The one who merged this pull request, just merge this pull request
> > >> without saying anything. I am not sure he mis-understand the
> > >> STATUS/READY-TO-MERGE label or not. But I think if read the context
> > >> through, one should be more cautious to merge that pull request.
> > >>
> > >> What I think can improve it:
> > >> - Every pull request must be approved officially by at least one of
> > >> the committer. (Comments such as LGTM does not mean official
> > >> approval.)
> > >> - If you feel uncertain about a pull request, please comment (see step
> > >> 6 in [2]) that you need more review to come in to review or request
> > >> review from others directly
> > >> - If you want to merge the pull request, you must read through all the
> > >> context.
> > >>
> > >> How do you think?
> > >>
> > >> [1] https://github.com/apache/incubator-dubbo/pull/3549
> > >> [2]
> > >>
> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
> > >>
> > >> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
> > >> >
> > >> > Folks,
> > >> >
> > >> > I think we need to revisit pull request 3549 in 2.7.2. In my
> opinion,
> > >> > fluent api need to be carefully resigned.
> > >> >
> > >> > What do you think?
> > >> >
> > >> > Thanks,
> > >> > -Ian.
> > >>
> > >>
> > >>
> > >> --
> > >> Best Regards！
> > >> Huxing
> > >>
>
>
>
> --
> Best Regards！
> Huxing
>
Re: Re: need to revisit pull request 3549 in 2.7.2
Posted by Huxing Zhang <hu...@apache.org>.
On Fri, Mar 15, 2019 at 4:31 PM kezhenxu94 <ke...@163.com> wrote:
>
> Hi,
>
>
> > - The one who merged this pull request, just merge this pull request
> > without saying anything. I am not sure he mis-understand the
> > STATUS/READY-TO-MERGE label or not. But I think if read the context
> > through, one should be more cautious to merge that pull request.
>
>
> I'm so sorry it was me who merged that pull request.
> I had read through the context(actually I reviewed although I did not comment)
> and indeed I've misunderstood the STATUS/READY-TO-MERGE label,
> and I suggest put more detail about the labels in the committer guide[1]
> to avoid later committers making the same mistakes.
Big +1 to that.
>
>
> Sorry again for making such mistake, I'll revert it if needed.
I think there is no need to revert unless there is some serious issues
(not looking very deeply though), one can improve it by sending
another pull request if possible.
>
>
>
>
> 1. http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html
>
>
>
>
>
>
>
>
>
>
> At 2019-03-15 16:28:16, "yuhang xiu" <ca...@gmail.com> wrote:
> >Hi, huxing.
> >
> >Great advice. In the previous review process, I also ignored some issues.
> >
> >Maybe we need a new label such as: `need-more-review`. When we need more
> >reviews, we can mark this pr to remind other committers to review it.
> >
> >In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I
> >think there is only one case where we will use this label, which is the
> >request that is approved, but this pr has not passed ci.
> >
> >In this pr merger, there are some problems, and we need to pay more
> >attention in the future.
> >
> >Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
> >
> >> Hi,
> >>
> >> Looking at the reviewing process of the pull request, I can see some
> >> issues here:
> >> - No one has officially approved the pull request before it getting
> >> merged. By saying officially I mean this[2], which I've mentioned in
> >> another thread.
> >> - One of the reviewer has marked the milestone as 2.7.2, but applied
> >> it with a label called STATUS/READY-TO-MERGE. These two activities
> >> look contradictory but according to the context he's meaning I guess
> >> is to put some more time to it.
> >> - The one who merged this pull request, just merge this pull request
> >> without saying anything. I am not sure he mis-understand the
> >> STATUS/READY-TO-MERGE label or not. But I think if read the context
> >> through, one should be more cautious to merge that pull request.
> >>
> >> What I think can improve it:
> >> - Every pull request must be approved officially by at least one of
> >> the committer. (Comments such as LGTM does not mean official
> >> approval.)
> >> - If you feel uncertain about a pull request, please comment (see step
> >> 6 in [2]) that you need more review to come in to review or request
> >> review from others directly
> >> - If you want to merge the pull request, you must read through all the
> >> context.
> >>
> >> How do you think?
> >>
> >> [1] https://github.com/apache/incubator-dubbo/pull/3549
> >> [2]
> >> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
> >>
> >> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
> >> >
> >> > Folks,
> >> >
> >> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> >> > fluent api need to be carefully resigned.
> >> >
> >> > What do you think?
> >> >
> >> > Thanks,
> >> > -Ian.
> >>
> >>
> >>
> >> --
> >> Best Regards！
> >> Huxing
> >>
--
Best Regards！
Huxing
Re:Re: need to revisit pull request 3549 in 2.7.2
Posted by kezhenxu94 <ke...@163.com>.
Hi,
> - The one who merged this pull request, just merge this pull request
> without saying anything. I am not sure he mis-understand the
> STATUS/READY-TO-MERGE label or not. But I think if read the context
> through, one should be more cautious to merge that pull request.
I'm so sorry it was me who merged that pull request.
I had read through the context(actually I reviewed although I did not comment)
and indeed I've misunderstood the STATUS/READY-TO-MERGE label,
and I suggest put more detail about the labels in the committer guide[1]
to avoid later committers making the same mistakes.
Sorry again for making such mistake, I'll revert it if needed.
1. http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html
At 2019-03-15 16:28:16, "yuhang xiu" <ca...@gmail.com> wrote:
>Hi, huxing.
>
>Great advice. In the previous review process, I also ignored some issues.
>
>Maybe we need a new label such as: `need-more-review`. When we need more
>reviews, we can mark this pr to remind other committers to review it.
>
>In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I
>think there is only one case where we will use this label, which is the
>request that is approved, but this pr has not passed ci.
>
>In this pr merger, there are some problems, and we need to pay more
>attention in the future.
>
>Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
>
>> Hi,
>>
>> Looking at the reviewing process of the pull request, I can see some
>> issues here:
>> - No one has officially approved the pull request before it getting
>> merged. By saying officially I mean this[2], which I've mentioned in
>> another thread.
>> - One of the reviewer has marked the milestone as 2.7.2, but applied
>> it with a label called STATUS/READY-TO-MERGE. These two activities
>> look contradictory but according to the context he's meaning I guess
>> is to put some more time to it.
>> - The one who merged this pull request, just merge this pull request
>> without saying anything. I am not sure he mis-understand the
>> STATUS/READY-TO-MERGE label or not. But I think if read the context
>> through, one should be more cautious to merge that pull request.
>>
>> What I think can improve it:
>> - Every pull request must be approved officially by at least one of
>> the committer. (Comments such as LGTM does not mean official
>> approval.)
>> - If you feel uncertain about a pull request, please comment (see step
>> 6 in [2]) that you need more review to come in to review or request
>> review from others directly
>> - If you want to merge the pull request, you must read through all the
>> context.
>>
>> How do you think?
>>
>> [1] https://github.com/apache/incubator-dubbo/pull/3549
>> [2]
>> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
>>
>> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
>> >
>> > Folks,
>> >
>> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
>> > fluent api need to be carefully resigned.
>> >
>> > What do you think?
>> >
>> > Thanks,
>> > -Ian.
>>
>>
>>
>> --
>> Best Regards！
>> Huxing
>>
Re: need to revisit pull request 3549 in 2.7.2
Posted by yuhang xiu <ca...@gmail.com>.
Hi, huxing.
Great advice. In the previous review process, I also ignored some issues.
Maybe we need a new label such as: `need-more-review`. When we need more
reviews, we can mark this pr to remind other committers to review it.
In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I
think there is only one case where we will use this label, which is the
request that is approved, but this pr has not passed ci.
In this pr merger, there are some problems, and we need to pay more
attention in the future.
Huxing Zhang <hu...@apache.org> 于2019年3月15日周五 下午4:03写道：
> Hi,
>
> Looking at the reviewing process of the pull request, I can see some
> issues here:
> - No one has officially approved the pull request before it getting
> merged. By saying officially I mean this[2], which I've mentioned in
> another thread.
> - One of the reviewer has marked the milestone as 2.7.2, but applied
> it with a label called STATUS/READY-TO-MERGE. These two activities
> look contradictory but according to the context he's meaning I guess
> is to put some more time to it.
> - The one who merged this pull request, just merge this pull request
> without saying anything. I am not sure he mis-understand the
> STATUS/READY-TO-MERGE label or not. But I think if read the context
> through, one should be more cautious to merge that pull request.
>
> What I think can improve it:
> - Every pull request must be approved officially by at least one of
> the committer. (Comments such as LGTM does not mean official
> approval.)
> - If you feel uncertain about a pull request, please comment (see step
> 6 in [2]) that you need more review to come in to review or request
> review from others directly
> - If you want to merge the pull request, you must read through all the
> context.
>
> How do you think?
>
> [1] https://github.com/apache/incubator-dubbo/pull/3549
> [2]
> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
>
> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
> >
> > Folks,
> >
> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> > fluent api need to be carefully resigned.
> >
> > What do you think?
> >
> > Thanks,
> > -Ian.
>
>
>
> --
> Best Regards！
> Huxing
>
Re: need to revisit pull request 3549 in 2.7.2
Posted by Huxing Zhang <hu...@apache.org>.
Hi,
Looking at the reviewing process of the pull request, I can see some
issues here:
- No one has officially approved the pull request before it getting
merged. By saying officially I mean this[2], which I've mentioned in
another thread.
- One of the reviewer has marked the milestone as 2.7.2, but applied
it with a label called STATUS/READY-TO-MERGE. These two activities
look contradictory but according to the context he's meaning I guess
is to put some more time to it.
- The one who merged this pull request, just merge this pull request
without saying anything. I am not sure he mis-understand the
STATUS/READY-TO-MERGE label or not. But I think if read the context
through, one should be more cautious to merge that pull request.
What I think can improve it:
- Every pull request must be approved officially by at least one of
the committer. (Comments such as LGTM does not mean official
approval.)
- If you feel uncertain about a pull request, please comment (see step
6 in [2]) that you need more review to come in to review or request
review from others directly
- If you want to merge the pull request, you must read through all the context.
How do you think?
[1] https://github.com/apache/incubator-dubbo/pull/3549
[2] https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews
On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <ia...@gmail.com> wrote:
>
> Folks,
>
> I think we need to revisit pull request 3549 in 2.7.2. In my opinion,
> fluent api need to be carefully resigned.
>
> What do you think?
>
> Thanks,
> -Ian.
--
Best Regards！
Huxing