+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.) 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 >>> >>> >>
