hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Allen Wittenauer ...@altiscale.com>
Subject Re: Patch review process
Date Fri, 30 Jan 2015 19:26:14 GMT

The fact that reviews.apache.org has ~35k users ( https://reviews.apache.org/users/?page=711
) that mostly appear to be bots gives me zero confidence in using this tool for anything real.


On Jan 30, 2015, at 11:11 AM, Gera Shegalov <gera@apache.org> wrote:

> Splitting the conversation via reviewboard and JIRA is definitely a problem
> that we have hit previously [1].
> 
> Since reviewboard and probably other tools as well generate emails for each
> set of comments we could leverage JIRA's functionality [2] to make sure
> that they are reflected in the JIRA as well. Probably there is some pre or
> post processing required to make sure that this happens. Of course since
> crucible delivers it out of the box[3], it should be the first candidate to
> look at. +1
> 
> 
> 1.
> https://issues.apache.org/jira/browse/MAPREDUCE-4974?focusedCommentId=13619455&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13619455
> ]
> 
> 2.
> https://confluence.atlassian.com/display/Cloud/Creating+Issues+and+Comments+from+Email
> 
> 3. https://www.atlassian.com/software/crucible/overview/code-quality-jira
> 
> On Thu Jan 29 2015 at 10:33:11 AM Colin P. McCabe <cmccabe@apache.org>
> wrote:
> 
>> I really do not think it's worth looking at Reviewboard at
>> reviews.apache.org again.  We have used it in the past, and it  has
>> all the downsides of gerrit and none of the upsides.  And some extra
>> downsides of its own.
>> 
>> * Splits the conversation into two places
>> * No way to search the split out conversation (no sophisticated query
>> language like JQL)
>> * An additional thing that new contributors have to learn
>> * A barrier to non-coders (since they don't post patches and so can't
>> contribute to the discussion there).
>> * Clunky UI (in my opinion)
>> * Requires manually filling in a long HTML form to upload a patch, or
>> using a fragile uploader script that often breaks when the reviewboard
>> software is updated
>> * No way to press a button and have a patch committed... the patch
>> commit process is just as time-consuming as it is now.
>> 
>> Sorry, but -1.
>> 
>> I like the Crucible idea, though.
>> 
>> If we want to investigate alternatives to Crucible, how about looking
>> at gerrit?  It has 1-click commits, integration with git (so that
>> posting a patch is just a single "git push"), and the potential to
>> mirror comments to JIRA (or at least someone said it might?)
>> 
>> Colin
>> 
>> 
>> On Tue, Jan 27, 2015 at 2:50 AM, Steve Loughran <stevel@hortonworks.com>
>> wrote:
>>> I'd be +1 on trying reviews.apache.org on a JIRA which
>>> 
>>>   1. had multiple distributed people working on it
>>>   2. had some tangible code needing reviewing
>>>   3. was of limited enough size/duration that we'd see how well it
>> worked
>>> 
>>> do that, get feedback from the participants and repeat until we're happy
>>> with a process.
>>> 
>>> if others can try cruicible at the same time, that would parallelise the
>>> work.
>>> 
>>> On 26 January 2015 at 22:41, Chris Nauroth <cnauroth@hortonworks.com>
>> wrote:
>>> 
>>>> reviews.apache.org is backed by Review Board, and I've had a very
>> positive
>>>> experience with that tool on other projects.  HADOOP-9629 is a Hadoop
>> patch
>>>> where we decided to go ahead and use it, and I think it helped.  AFAIK,
>>>> there is no rule against using it in Hadoop, but it does have the side
>>>> effect of splitting part of the conversation out of jira.  If Crucible
>> can
>>>> keep all the review notes sync'd with the jira and searchable, then that
>>>> would be very interesting.
>>>> 
>>>> Chris Nauroth
>>>> Hortonworks
>>>> http://hortonworks.com/
>>>> 
>>>> 
>>>> On Mon, Jan 26, 2015 at 1:54 PM, Arpit Agarwal <
>> aagarwal@hortonworks.com>
>>>> wrote:
>>>> 
>>>>> IMO the number one improvement would be a web-based review tool. We
>> could
>>>>> evaluate Atlassian Crucible since it claims to integrate well with
>> Jira.
>>>> I
>>>>> have not tried https://reviews.apache.org/r/new/.
>>>>> 
>>>>> Some easy improvements that were also raised by others on the private
>>>> list:
>>>>> - Encourage contributors to batch related trivial fixes into a single
>>>>> patch.
>>>>> - Require more detailed descriptions with non-trivial patch
>>>> contributions.
>>>>> For patches that require knowledge of a specific subsystem a
>>>>> background+design note would be a good start.
>>>>> - Eliminate CHANGES.txt. This came up on the dev list not too long ago
>>>> and
>>>>> IIRC Allen did a PoC.
>>>>> 
>>>>> I am not optimistic about Gerrit from past experience. It does help
>> gate
>>>>> checkins and enforce pre-commit checks (good). I did not find it
>>>>> user-friendly and it will be an additional hurdle for contributors to
>>>>> understand (bad).
>>>>> 
>>>>> Andrew, can the community build on your distributed pre-commit work to
>>>> make
>>>>> it production ready?
>>>>> 
>>>>> Regards,
>>>>> Arpit
>>>>> 
>>>>> 
>>>>> On Mon, Jan 26, 2015 at 11:55 AM, Andrew Wang <
>> andrew.wang@cloudera.com>
>>>>> wrote:
>>>>> 
>>>>>> Let's move this over to common-dev@, general@ is normally used for
>>>>> project
>>>>>> announcements rather than discussion topics.
>>>>>> 
>>>>>> I'd like to summarize a few things mentioned on the private@
>> thread,
>>>>>> related to streamlining the code submission process.
>>>>>> 
>>>>>> - Gerrit was brought up again, as it has in the past, as something
>> that
>>>>>> could make the actual process of reviewing and committing a lot
>> easier.
>>>>>> This would be especially helpful for small patches, where the
>> mechanics
>>>>> of
>>>>>> committing can take longer than reviewing the patch.
>>>>>> - There were also concerns about forking discussions between JIRA
>> and
>>>>>> Gerrit. This has been an issue in Spark, and we'd like to keep
>>>>> discussions
>>>>>> and issue tracking centralized.
>>>>>> 
>>>>>> - Some talk about how to improve precommit. Right now it takes
>> hours to
>>>>> run
>>>>>> the unit tests, which slows down patch iterations. One solution is
>>>>> running
>>>>>> tests in parallel (and even distributed). Previous distributed
>>>>> experiments
>>>>>> have done a full unit test run in a couple minutes, but it'd be a
>> fair
>>>>>> amount of work to actually make this production ready.
>>>>>> - Also mention of putting in place more linting and static analysis.
>>>>>> Automating this will save reviewer time.
>>>>>> 
>>>>>> Best,
>>>>>> Andrew
>>>>>> 
>>>>>> On Mon, Jan 26, 2015 at 9:16 AM, Ted Yu <yuzhihong@gmail.com>
>> wrote:
>>>>>> 
>>>>>>> In some cases, contributor responded to review comments and
>> attached
>>>>>>> patches addressing the comments.
>>>>>>> 
>>>>>>> Later on, there was simply no response to the latest patch -
even
>>>> with
>>>>>>> follow-on ping.
>>>>>>> 
>>>>>>> I wish this aspect can be improved.
>>>>>>> 
>>>>>>> Cheers
>>>>>>> 
>>>>>>> On Sun, Jan 25, 2015 at 6:03 PM, Tsz Wo (Nicholas), Sze <
>>>>>>> s29752-hadoopgeneral@yahoo.com.invalid> wrote:
>>>>>>> 
>>>>>>>> Hi contributors,
>>>>>>>> I would like to (re)start a discussion regrading to our patch
>>>> review
>>>>>>>> process.  A similar discussion has been happened in a the
hadoop
>>>>>> private
>>>>>>>> mailing list, which is inappropriate.
>>>>>>>> Here is the problem:The patch available queues become longer
and
>>>>>> longer.
>>>>>>>> It seems that we never can catch up.  There are patches sitting
>> in
>>>>> the
>>>>>>>> queues for years.  How could we speed up?
>>>>>>>> Regrads,Tsz-Wo
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>>> --
>>>>> CONFIDENTIALITY NOTICE
>>>>> NOTICE: This message is intended for the use of the individual or
>> entity
>>>> to
>>>>> which it is addressed and may contain information that is
>> confidential,
>>>>> privileged and exempt from disclosure under applicable law. If the
>> reader
>>>>> of this message is not the intended recipient, you are hereby notified
>>>> that
>>>>> any printing, copying, dissemination, distribution, disclosure or
>>>>> forwarding of this communication is strictly prohibited. If you have
>>>>> received this communication in error, please contact the sender
>>>> immediately
>>>>> and delete it from your system. Thank You.
>>>>> 
>>>> 
>>>> --
>>>> CONFIDENTIALITY NOTICE
>>>> NOTICE: This message is intended for the use of the individual or
>> entity to
>>>> which it is addressed and may contain information that is confidential,
>>>> privileged and exempt from disclosure under applicable law. If the
>> reader
>>>> of this message is not the intended recipient, you are hereby notified
>> that
>>>> any printing, copying, dissemination, distribution, disclosure or
>>>> forwarding of this communication is strictly prohibited. If you have
>>>> received this communication in error, please contact the sender
>> immediately
>>>> and delete it from your system. Thank You.
>>>> 
>>> 
>>> --
>>> CONFIDENTIALITY NOTICE
>>> NOTICE: This message is intended for the use of the individual or entity
>> to
>>> which it is addressed and may contain information that is confidential,
>>> privileged and exempt from disclosure under applicable law. If the reader
>>> of this message is not the intended recipient, you are hereby notified
>> that
>>> any printing, copying, dissemination, distribution, disclosure or
>>> forwarding of this communication is strictly prohibited. If you have
>>> received this communication in error, please contact the sender
>> immediately
>>> and delete it from your system. Thank You.
>> 


Mime
View raw message