hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Todd Lipcon <t...@cloudera.com>
Subject Re: patch review process
Date Thu, 20 Jan 2011 20:16:51 GMT
On Thu, Jan 20, 2011 at 12:12 PM, John Sichi <jsichi@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 <heyongqiangict@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 <njain@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message