hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Namit Jain <>
Subject Re: patch review process
Date Thu, 20 Jan 2011 22:56:04 GMT
I agree on the problem of comments.
For a new reviewer, it makes the process more painful.

But, I think, the issues are not major, and we should
decide one approach or another.

Even for option 2., we can make it mandatory for the contributor
to submit a reviewboard request if the reviewer asks for it.

Can others vote please ?


On 1/20/11 2:25 PM, "yongqiang he" <> wrote:

>I would argue that how to do the review has nothing to do with code
>quality. No review board has no connection with low quality code.
>The review board just provides a limited view of the diff's context.
>So if the review is familiar with the diff's context and is confident
>with the patch, it is his call to decide the review process. And also
>even with the review board, the reviewer sometimes still need to open
>his workspace to look for more contexts, like the references and the
>caller of a function etc.
>The reason that option 2 looks good to me are the rule here is just
>assuming people in this community acting nice to each other. The
>committer/reviewer creates a review board for the contributor because
>he is nice to the contributor, right? And the contributor should also
>create review board for following patches in the same issue because he
>knows the reviewer needs a review board to review his patch, and he
>should be nice to the reviewer by doing that himself.
>Another thing came up from an offline discussion is that are the
>comments on review board coming back to the jira? If no, that means
>the comments are not searchable, and the later followers, who want to
>get a whole understanding of the problem/solutions/pitfalls, need to
>open the patch/review board page to find all the comments from
>And I want to clear that I am not agains review board. I would suggest
>let us move on with what each one feels comfortable and more
>productable, and avoid enforcing some rules on this.
>On Thu, Jan 20, 2011 at 12:16 PM, Todd Lipcon <> wrote:
>> On Thu, Jan 20, 2011 at 12:12 PM, John Sichi <> wrote:
>>> +1 on option 1.  This is standard operating practice (for all code
>>> no exceptions) at Facebook, Google, and many other companies that care
>>> 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
>>> to squash stuff into one file, e.g. via inner class, instead of
>>> 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
>> trivial things (eg a doc update or spelling fix or something of that
>>> 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
>>> be able to just give it a HIVE-xyz JIRA number, and the script would
>>> the necessary information and include that in the postreview
>>>  There should be very few cases where anyone needs to go through the
>>> Board UI.  See this section and following for more:
>>> (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,
>>> would automatically pull the patch from review board so that the
>>> does not have to do a separate patch-generation + upload.
>>> (3) Eliminate all generated code from the codebase; this causes a lot
>>> extra friction.  Now that we have moved to an official thrift release,
>>> 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
>>> the
>>> > reviewer. If you want to look at a patch you have to download it,
>>> it
>>> > to a clean workspace, view it using the diff viewer of your choice,
>>> 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
>>> > setup steps that are currently incumbent on the reviewer and
>>> > 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
>>> > tools like 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
>>> > 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
>>> 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
>>> for
>>> > the contributor to create the request since only the creator can
>>> it.
>>> >
>>> > Thanks.
>>> >
>>> > Carl
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > On Wed, Jan 19, 2011 at 6:48 PM, yongqiang he
>>> >wrote:
>>> >
>>> >> +1 for option 2.
>>> >>
>>> >> In general, we as a community should be nice to all contributors,
>>> >> should avoid doing things that make contributors not comfortable,
>>> >> 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 <> 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,
>>> >> large,
>>> >>> a GUI like reviewboard ( 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
>>> >>>
>>> >>>
>>> >>> 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

View raw message