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
