Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <AANLkTimctv0w9Bz9B5nHm0V7o8O2KD6+ac3jbZcoGBqb@mail.gmail.com>
References: <C95CBAF4.1CCB4%njain@fb.com>
	<C95CDFE5.1CCE5%njain@fb.com>
	<AANLkTimctv0w9Bz9B5nHm0V7o8O2KD6+ac3jbZcoGBqb@mail.gmail.com>
Date: Wed, 19 Jan 2011 21:05:56 -0800
Message-ID: <AANLkTim17d7EJBzDQd0SJgJ0R_rpv3JAq2E0VUzM83y9@mail.gmail.com>
Subject: Re: patch review process
From: Carl Steinbach <carl@cloudera.com>
To: dev@hive.apache.org
Content-Type: multipart/alternative; boundary=0016e6d588d7b9e343049a40161f

--0016e6d588d7b9e343049a40161f
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

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 the=
n
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 reques=
t
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>wro=
te:

> +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=92t rea=
lly
> 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
> >
> >
>

--0016e6d588d7b9e343049a40161f--