The system that we have in place right now places all of the burden on the reviewer. If you want to look at a patch you have to download it, apply it to a clean workspace, view it using the diff viewer of your choice, and then copy your comments back to JIRA along with line numbers and code fragments in order to provide context for the author. If there's more than one reviewer, then everyone repeats these steps individually. From this perspective I think using ReviewBoard is a clear win. It eliminates the setup steps that are currently incumbent on the reviewer and consequently encourages more people to participate in the review process, which I think will result in higher quality code in the end.
I think that the additional burden that ReviewBoard places on the contributor is very small (especially when compared to the effort invested in producing the patch in the first place) and can be mitigated by using tools like post-review ( http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/). I'm +1 for option (1), meaning that I think people should be required to post a review request (or update an existing request) for every patch that they submit for review on JIRA. I also think excluding small patches from this requirement is a bad idea because rational people can disagree about what qualifies as a small patch and what does not, and I'd like people to make ReviewBoard a habit instead of something that they use occasionally. I think that Yongqiang's point about scaring away new contributors with lots of requirements is valid, and I'm more that willing to post a review request for a first (or second) time contributor, but in general it's important for the contributor to create the request since only the creator can update it. Thanks. Carl On Wed, Jan 19, 2011 at 6:48 PM, yongqiang he <heyongqiang...@gmail.com>wrote: > +1 for option 2. > > In general, we as a community should be nice to all contributors, and > should avoid doing things that make contributors not comfortable, even > that requires some work from committers. Sometimes it is especially > true for new contributors, like we need to be more patience for new > people. It seems a free style and contribution focused environment > would be better to encourage people to do more contributions of > different kinds. > > thanks > -yongqiang > On Wed, Jan 19, 2011 at 6:37 PM, Namit Jain <nj...@fb.com> wrote: > > > > > > > > It would be good to have a policy for submitting a new patch for review. > > If the patch is small, usually it is pretty easy to review.But, if it > large, > > a GUI like reviewboard (https://reviews.apache.org) makes it easy. > > > > So, going forward, I would like to propose either of the following. > > > > 1. All patches must go through reviewboard > > 2. If a contributor/reviewer creates a reviewboard request, > > all subsequent review requests should go through the reviewboard. > > > > > > I would personally vote for 2., since for small patches, we don’t really > need a > > reviewboard. > > > > But, please vote, and based on that, we can come up with a policy. > > Let us know, if you think of some other option. > > > > Thanks, > > -namit > > > > >
