You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Thomas Koch <th...@koch.ro> on 2011/09/22 11:22:50 UTC
Make ReviewBoard obligatory?
Hi,
as Camille suggested[1], I've used ReviewBoard in the last weeks for a couple
of issues. I believe it's a very good tool and helps a lot. Actually I ask
myself, how one can do an effective code review without such a tool? It's kind
of time-consuming to download the patch file, inspect it in an editor and post
comments to jira, copy and pasting code lines or typing line numbers.
What do you think? Would it be good to strongly encourage the use of
ReviewBoard for every change whose patch file is longer then ~30 lines? I also
think, that the current process of using ReviewBoard is time-consuming. But if
that should be the reason to reject a review tool, then you might have a look
to my suggestion of using Gerrit at the ASF[2].
I scanned the wiki[3][4] and didn't find ReviewBoard mentioned. ZOOKEEPER-1172
is an example of an (I believe) new contributor, who didn't know about
ReviewBoard and also didn't correctly fill the ReviewRequest. I believe that
the review process could become easier for the committers, if people would
default to open review requests.
[1] http://comments.gmane.org/gmane.comp.java.hadoop.zookeeper.devel/10095
[2] http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361
[3] https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
[4] https://cwiki.apache.org/confluence/display/ZOOKEEPER/Committing+changes
Regards,
Thomas Koch, http://www.koch.ro
Re: Make ReviewBoard obligatory?
Posted by Patrick Hunt <ph...@apache.org>.
Yes, I suspect that's the case. We should call this out explicitly in
the "how to contribute" changes for RB.
Also, using the rbtools I mentioned helps with this.
Patrick
On Thu, Sep 22, 2011 at 11:35 AM, Eugene Koontz <ek...@hiro-tan.org> wrote:
> On 9/22/11 10:12 AM, Patrick Hunt wrote:
>>
>> Eugene - already there:
>>
>> https://issues.apache.org/jira/browse/ZOOKEEPER-1107?focusedCommentId=13067981&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13067981
>>
>> On Thu, Sep 22, 2011 at 9:10 AM, Eugene Koontz<ek...@hiro-tan.org>
>> wrote:
>>>
>>> It would be helpful if the JIRAs reflect changes made on their respective
>>> reviews. For example, with HBASE, when someone comments on a review or
>>> uploads a new review to the review board, a comment automatically is
>>> posted
>>> to the JIRA by " jiraposter@reviews.apache.org"
>>>
>>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=jiraposter%40reviews.apache.org>
>>> . For example: https://issues.apache.org/jira/browse/HBASE-4014
>>>
>>> -Eugene
>>>
> Thanks Patrick,
>
> I was wondering why my review update to https://reviews.apache.org/r/1959/
> did not appear on the corresponding JIRA:
> https://issues.apache.org/jira/browse/ZOOKEEPER-1185
>
> I think it was that I forgot to fill out the "Bugs" field in the review.
> Fixed that now.
>
> -Eugene
>
Re: Make ReviewBoard obligatory?
Posted by Eugene Koontz <ek...@hiro-tan.org>.
On 9/22/11 10:12 AM, Patrick Hunt wrote:
> Eugene - already there:
> https://issues.apache.org/jira/browse/ZOOKEEPER-1107?focusedCommentId=13067981&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13067981
>
> On Thu, Sep 22, 2011 at 9:10 AM, Eugene Koontz<ek...@hiro-tan.org> wrote:
>> It would be helpful if the JIRAs reflect changes made on their respective
>> reviews. For example, with HBASE, when someone comments on a review or
>> uploads a new review to the review board, a comment automatically is posted
>> to the JIRA by " jiraposter@reviews.apache.org"
>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=jiraposter%40reviews.apache.org>
>> . For example: https://issues.apache.org/jira/browse/HBASE-4014
>>
>> -Eugene
>>
Thanks Patrick,
I was wondering why my review update to
https://reviews.apache.org/r/1959/ did not appear on the corresponding
JIRA:
https://issues.apache.org/jira/browse/ZOOKEEPER-1185
I think it was that I forgot to fill out the "Bugs" field in the review.
Fixed that now.
-Eugene
Re: Make ReviewBoard obligatory?
Posted by Patrick Hunt <ph...@apache.org>.
Eugene - already there:
https://issues.apache.org/jira/browse/ZOOKEEPER-1107?focusedCommentId=13067981&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13067981
On Thu, Sep 22, 2011 at 9:10 AM, Eugene Koontz <ek...@hiro-tan.org> wrote:
> It would be helpful if the JIRAs reflect changes made on their respective
> reviews. For example, with HBASE, when someone comments on a review or
> uploads a new review to the review board, a comment automatically is posted
> to the JIRA by " jiraposter@reviews.apache.org"
> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=jiraposter%40reviews.apache.org>
> . For example: https://issues.apache.org/jira/browse/HBASE-4014
>
> -Eugene
>
Re: Make ReviewBoard obligatory?
Posted by Eugene Koontz <ek...@hiro-tan.org>.
It would be helpful if the JIRAs reflect changes made on their
respective reviews. For example, with HBASE, when someone comments on a
review or uploads a new review to the review board, a comment
automatically is posted to the JIRA by " jiraposter@reviews.apache.org"
<https://issues.apache.org/jira/secure/ViewProfile.jspa?name=jiraposter%40reviews.apache.org>
. For example: https://issues.apache.org/jira/browse/HBASE-4014
-Eugene
Re: Make ReviewBoard obligatory?
Posted by Patrick Hunt <ph...@apache.org>.
On Thu, Sep 22, 2011 at 10:23 AM, Mahadev Konar <ma...@hortonworks.com> wrote:
> I think review board is definitely a good practice but we probably
> dont have to make it mandatory. We can definitely update the
> howtocontribute twiki on uploading to review board, if the patch is
> big enough. I usually dont use review board. I download the patch and
> use eclipse to see what changes have been made (mostly because
> sometimes I just edit the patch myself).
I personally find it useful wrt providing f/b to the contributor, more
than as a review tool in and of itself. It also makes it easy to see
the changes btw patch versions (say you go back/forth with the
contributor).
Regardless I'm +1 generally to adding RB as part of our contribution process.
Patrick
Re: Make ReviewBoard obligatory?
Posted by Mahadev Konar <ma...@hortonworks.com>.
I think review board is definitely a good practice but we probably
dont have to make it mandatory. We can definitely update the
howtocontribute twiki on uploading to review board, if the patch is
big enough. I usually dont use review board. I download the patch and
use eclipse to see what changes have been made (mostly because
sometimes I just edit the patch myself).
thanks
mahadev
On Thu, Sep 22, 2011 at 7:25 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> I think it's great to encourage people to use it if they feel the need or desire. Certainly for longer patches (new features especially) where you have a lot of comments to make, reviewboard is useful. However, everyone has their own workflow. For me, I always download every patch I review and inspect it in my editor, along with running a subset of related tests. If I then have comments to make about individual lines, I'll often upload it to RB to make the comments, but frequently the comments are either of a more general nature (this doesn't solve the bug it purports to address) or there are no comments necessary. Experience has shown me that trying to do good code reviews without actually looking at the patch in the context of my IDE is error
>
> What I'm not clear on is why you want to make a rule that we must do this. Is there some larger problem you see that you think this would help us solve? Are reviews not transparent enough? Is the process too slow? Are we missing errors because we don't have good review tools? You seem to be proposing a solution to a problem that no one has complained about.
>
> Anyway, we should certainly update the wiki to explain how to use RB and when it is recommended to do so (such as when proposing a new feature). I think you should be able to do this, if you are willing.
>
>
>
> -----Original Message-----
> From: Thomas Koch [mailto:thomas@koch.ro]
> Sent: Thursday, September 22, 2011 5:23 AM
> To: dev@zookeeper.apache.org
> Subject: Make ReviewBoard obligatory?
>
> Hi,
>
> as Camille suggested[1], I've used ReviewBoard in the last weeks for a couple
> of issues. I believe it's a very good tool and helps a lot. Actually I ask
> myself, how one can do an effective code review without such a tool? It's kind
> of time-consuming to download the patch file, inspect it in an editor and post
> comments to jira, copy and pasting code lines or typing line numbers.
>
> What do you think? Would it be good to strongly encourage the use of
> ReviewBoard for every change whose patch file is longer then ~30 lines? I also
> think, that the current process of using ReviewBoard is time-consuming. But if
> that should be the reason to reject a review tool, then you might have a look
> to my suggestion of using Gerrit at the ASF[2].
>
> I scanned the wiki[3][4] and didn't find ReviewBoard mentioned. ZOOKEEPER-1172
> is an example of an (I believe) new contributor, who didn't know about
> ReviewBoard and also didn't correctly fill the ReviewRequest. I believe that
> the review process could become easier for the committers, if people would
> default to open review requests.
>
> [1] http://comments.gmane.org/gmane.comp.java.hadoop.zookeeper.devel/10095
> [2] http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361
> [3] https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
> [4] https://cwiki.apache.org/confluence/display/ZOOKEEPER/Committing+changes
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
RE: Make ReviewBoard obligatory?
Posted by "Fournier, Camille F." <Ca...@gs.com>.
I think it's great to encourage people to use it if they feel the need or desire. Certainly for longer patches (new features especially) where you have a lot of comments to make, reviewboard is useful. However, everyone has their own workflow. For me, I always download every patch I review and inspect it in my editor, along with running a subset of related tests. If I then have comments to make about individual lines, I'll often upload it to RB to make the comments, but frequently the comments are either of a more general nature (this doesn't solve the bug it purports to address) or there are no comments necessary. Experience has shown me that trying to do good code reviews without actually looking at the patch in the context of my IDE is error
What I'm not clear on is why you want to make a rule that we must do this. Is there some larger problem you see that you think this would help us solve? Are reviews not transparent enough? Is the process too slow? Are we missing errors because we don't have good review tools? You seem to be proposing a solution to a problem that no one has complained about.
Anyway, we should certainly update the wiki to explain how to use RB and when it is recommended to do so (such as when proposing a new feature). I think you should be able to do this, if you are willing.
-----Original Message-----
From: Thomas Koch [mailto:thomas@koch.ro]
Sent: Thursday, September 22, 2011 5:23 AM
To: dev@zookeeper.apache.org
Subject: Make ReviewBoard obligatory?
Hi,
as Camille suggested[1], I've used ReviewBoard in the last weeks for a couple
of issues. I believe it's a very good tool and helps a lot. Actually I ask
myself, how one can do an effective code review without such a tool? It's kind
of time-consuming to download the patch file, inspect it in an editor and post
comments to jira, copy and pasting code lines or typing line numbers.
What do you think? Would it be good to strongly encourage the use of
ReviewBoard for every change whose patch file is longer then ~30 lines? I also
think, that the current process of using ReviewBoard is time-consuming. But if
that should be the reason to reject a review tool, then you might have a look
to my suggestion of using Gerrit at the ASF[2].
I scanned the wiki[3][4] and didn't find ReviewBoard mentioned. ZOOKEEPER-1172
is an example of an (I believe) new contributor, who didn't know about
ReviewBoard and also didn't correctly fill the ReviewRequest. I believe that
the review process could become easier for the committers, if people would
default to open review requests.
[1] http://comments.gmane.org/gmane.comp.java.hadoop.zookeeper.devel/10095
[2] http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361
[3] https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
[4] https://cwiki.apache.org/confluence/display/ZOOKEEPER/Committing+changes
Regards,
Thomas Koch, http://www.koch.ro
Re: Make ReviewBoard obligatory?
Posted by Patrick Hunt <ph...@apache.org>.
I would be in favor of requiring RB for any/all changes, regardless of
size. We do this at Cloudera religiously, I find it's a good practice
and it eliminates any guesswork.
We also use rbtools (specifically post-review.py), which greatly
simplifies the process of posting a review.
http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/
Patrick
On Thu, Sep 22, 2011 at 2:22 AM, Thomas Koch <th...@koch.ro> wrote:
> Hi,
>
> as Camille suggested[1], I've used ReviewBoard in the last weeks for a couple
> of issues. I believe it's a very good tool and helps a lot. Actually I ask
> myself, how one can do an effective code review without such a tool? It's kind
> of time-consuming to download the patch file, inspect it in an editor and post
> comments to jira, copy and pasting code lines or typing line numbers.
>
> What do you think? Would it be good to strongly encourage the use of
> ReviewBoard for every change whose patch file is longer then ~30 lines? I also
> think, that the current process of using ReviewBoard is time-consuming. But if
> that should be the reason to reject a review tool, then you might have a look
> to my suggestion of using Gerrit at the ASF[2].
>
> I scanned the wiki[3][4] and didn't find ReviewBoard mentioned. ZOOKEEPER-1172
> is an example of an (I believe) new contributor, who didn't know about
> ReviewBoard and also didn't correctly fill the ReviewRequest. I believe that
> the review process could become easier for the committers, if people would
> default to open review requests.
>
> [1] http://comments.gmane.org/gmane.comp.java.hadoop.zookeeper.devel/10095
> [2] http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361
> [3] https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
> [4] https://cwiki.apache.org/confluence/display/ZOOKEEPER/Committing+changes
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
Re: Make ReviewBoard obligatory?
Posted by Benjamin Reed <br...@apache.org>.
yes, i agree, and ~30 lines does sound like a good threshold. we
should update the wikis. the initial dialog box was a bit daunting the
first time since i couldn't figured out what they meant by base
directory and then i couldn't figure out how i should name it.
documenting the policy, procedures, and conventions would make it much
easier all the way around.
ben
On Thu, Sep 22, 2011 at 2:22 AM, Thomas Koch <th...@koch.ro> wrote:
> Hi,
>
> as Camille suggested[1], I've used ReviewBoard in the last weeks for a couple
> of issues. I believe it's a very good tool and helps a lot. Actually I ask
> myself, how one can do an effective code review without such a tool? It's kind
> of time-consuming to download the patch file, inspect it in an editor and post
> comments to jira, copy and pasting code lines or typing line numbers.
>
> What do you think? Would it be good to strongly encourage the use of
> ReviewBoard for every change whose patch file is longer then ~30 lines? I also
> think, that the current process of using ReviewBoard is time-consuming. But if
> that should be the reason to reject a review tool, then you might have a look
> to my suggestion of using Gerrit at the ASF[2].
>
> I scanned the wiki[3][4] and didn't find ReviewBoard mentioned. ZOOKEEPER-1172
> is an example of an (I believe) new contributor, who didn't know about
> ReviewBoard and also didn't correctly fill the ReviewRequest. I believe that
> the review process could become easier for the committers, if people would
> default to open review requests.
>
> [1] http://comments.gmane.org/gmane.comp.java.hadoop.zookeeper.devel/10095
> [2] http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361
> [3] https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
> [4] https://cwiki.apache.org/confluence/display/ZOOKEEPER/Committing+changes
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>