I was thinking about it. Let us do the following: For big patches (a big patch is one which involves changing more than 10 files - those files can be test/code changes or even generated files), reviewBoard is mandatory.
For smaller patches, reviewBoard is optional. However, if any reviewer asks for a reviewBoard request from the committer, it becomes mandatory, and the contributor needs to create the reviewBoard request. As good practice, also copy important review comments in the jira, so that they are searchable and give enough context to a new contributor. Thanks, -namit On 1/20/11 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/#repos >>itory-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
