I would argue that how to do the review has nothing to do with code quality. No review board has no connection with low quality code.
The review board just provides a limited view of the diff's context. So if the review is familiar with the diff's context and is confident with the patch, it is his call to decide the review process. And also even with the review board, the reviewer sometimes still need to open his workspace to look for more contexts, like the references and the caller of a function etc. The reason that option 2 looks good to me are the rule here is just assuming people in this community acting nice to each other. The committer/reviewer creates a review board for the contributor because he is nice to the contributor, right? And the contributor should also create review board for following patches in the same issue because he knows the reviewer needs a review board to review his patch, and he should be nice to the reviewer by doing that himself. Another thing came up from an offline discussion is that are the comments on review board coming back to the jira? If no, that means the comments are not searchable, and the later followers, who want to get a whole understanding of the problem/solutions/pitfalls, need to open the patch/review board page to find all the comments from reviewers. And I want to clear that I am not agains review board. I would suggest let us move on with what each one feels comfortable and more productable, and avoid enforcing some rules on this. thanks -yongqiang On Thu, Jan 20, 2011 at 12:16 PM, Todd Lipcon <t...@cloudera.com> wrote: > On Thu, Jan 20, 2011 at 12:12 PM, John Sichi <jsi...@fb.com> wrote: > >> +1 on option 1. This is standard operating practice (for all code changes, >> no exceptions) at Facebook, Google, and many other companies that care about >> code quality. >> >> (The latest HBase wiki makes an exception for patches that only involve one >> changed+unreviewed file, but I think that creates a bad incentive for people >> to squash stuff into one file, e.g. via inner class, instead of properly >> refactoring in cases where it is called for.) >> > > huh, I had no idea that was the case for HBase :) > > We generally follow the policy that you can Commit-Then-Review for obviously > trivial things (eg a doc update or spelling fix or something of that sort) > > >> >> To better facilitate this policy, we should make sure that the workflow is >> as smooth as possible. >> >> (1) Make usage of postreview much more pominent in the HowToContribute >> docs, and do everything possible to raise awareness about its existence. I >> think we should also configure our repositories and check in a wrapper >> script in order to make post-review usage a no-brainer: ideally, we would >> be able to just give it a HIVE-xyz JIRA number, and the script would wget >> the necessary information and include that in the postreview submission. >> There should be very few cases where anyone needs to go through the Review >> Board UI. See this section and following for more: >> http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/#repository-configuration >> >> (2) See if we can configure JIRA to require a review board link as part of >> submitting a patch. This makes the system self-enforcing. Ideally, JIRA >> would automatically pull the patch from review board so that the contributor >> does not have to do a separate patch-generation + upload. >> >> (3) Eliminate all generated code from the codebase; this causes a lot of >> extra friction. Now that we have moved to an official thrift release, this >> should be easier. >> >> If we do all of these, I think the net result will actually be a better >> experience for contributors relative to what we have today. >> >> JVS >> >> On Jan 19, 2011, at 9:05 PM, Carl Steinbach wrote: >> >> > 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 >> >>> >> >>> >> >> >> >> > > > -- > Todd Lipcon > Software Engineer, Cloudera >
