|
|
-
Make ReviewBoard obligatory?
Thomas Koch 2011-09-22, 09:22
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+changesRegards, Thomas Koch, http://www.koch.ro
+
Thomas Koch 2011-09-22, 09:22
-
Re: Make ReviewBoard obligatory?
Benjamin Reed 2011-09-22, 14:21
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 <[EMAIL PROTECTED]> 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>
+
Benjamin Reed 2011-09-22, 14:21
-
RE: Make ReviewBoard obligatory?
Fournier, Camille F. 2011-09-22, 14:25
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:[EMAIL PROTECTED]] Sent: Thursday, September 22, 2011 5:23 AM To: [EMAIL PROTECTED] 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+changesRegards, Thomas Koch, http://www.koch.ro
+
Fournier, Camille F. 2011-09-22, 14:25
+
Eugene Koontz 2011-09-22, 16:10
+
Patrick Hunt 2011-09-22, 17:12
+
Eugene Koontz 2011-09-22, 18:35
-
Re: Make ReviewBoard obligatory?
Patrick Hunt 2011-09-22, 18:59
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 <[EMAIL PROTECTED]> 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<[EMAIL PROTECTED]> >> 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 " [EMAIL PROTECTED]" >>> >>> < 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 >
+
Patrick Hunt 2011-09-22, 18:59
-
Re: Make ReviewBoard obligatory?
Mahadev Konar 2011-09-22, 17:23
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. <[EMAIL PROTECTED]> 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:[EMAIL PROTECTED]] > Sent: Thursday, September 22, 2011 5:23 AM > To: [EMAIL PROTECTED] > 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>
+
Mahadev Konar 2011-09-22, 17:23
-
Re: Make ReviewBoard obligatory?
Patrick Hunt 2011-09-22, 17:27
On Thu, Sep 22, 2011 at 10:23 AM, Mahadev Konar <[EMAIL PROTECTED]> 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
+
Patrick Hunt 2011-09-22, 17:27
-
Re: Make ReviewBoard obligatory?
Patrick Hunt 2011-09-22, 17:16
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 <[EMAIL PROTECTED]> 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>
+
Patrick Hunt 2011-09-22, 17:16
|
|
