arrow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wes McKinney <...@cloudera.com>
Subject Re: Code review tools for Arrow patches
Date Sun, 24 Apr 2016 15:30:59 GMT
Todd, would you (or someone who is familiar with Gerrit
administration) mind setting up gerrit.cloudera.org projects that we
can use for apache/arrow and apache/parquet-cpp? We're having more
larger patches to both projects and I'm having a difficult time
staying organized in my reviews on GitHub (especially reviewing
updated patch sets). I'll provide instructions for using these
instances as an optional review tool (will definitely not be required
as part of the patch sign-off procedure).

Thank you
Wes

On Fri, Apr 15, 2016 at 3:48 PM, Wes McKinney <wes@cloudera.com> wrote:
> hey Jason,
>
> I have not used Reviewboard, but the problems you are describing are
> (AFAICT) not common complaint among Gerrit users. It would be helpful
> to hear more from experienced Gerrit users.
>
> Note that my initial request was not "let's choose a tool that is not
> GitHub PRs" for code reviews but rather that "let's have more tools"
> -- I'm more than happy if groups of committers choose the tool that
> works best for them, and we proceed in a somewhat anarchic fashion for
> the time being. I think using PRs as training wheels for newcomers is
> perfectly acceptable and when the patches grow more extensive,
> reviewers can make a judgment call when using a more advanced tool (I
> would describe Gerrit as requiring somewhat advanced git expertise,
> rebasing and so forth) makes sense.
>
> - Wes
>
> On Fri, Apr 15, 2016 at 7:34 PM, Jason Altekruse
> <altekrusejason@gmail.com> wrote:
>> It looks like I am going to be a minority opinion here, but I think there
>> is at least a case to make that pull requests area little easier for
>> newcomers.
>> I also have opinions about rebasing branches that are shared publicly or
>> currently under review. While it isn't often a problem, rebasing often
>> involves squashing which further distorts the history of how a feature was
>> developed. This is part of the reason why we had moved to Pull requests for
>> Drill. We reached a juncture where we were making fixes and implementing
>> features often required refactoring or general cleanup, and the reviews got
>> messy on reviewboard. Pull requests make it easier to review a multi-part
>> changeset, where cosmetic or refactoring changes can be in separate commit
>> from bug fixes or features. The github interface does give you decent
>> access to view individual commits, or the combination of all of the work on
>> the branch.
>>
>> I have only been working with gerrit recently, but it seems to have the
>> same behavior or reviewboard in terms of providing a mechanism for looking
>> at essentially diffs between diffs. The relationship between these overall
>> changeset revisions can get quite complicated, as the first one will be
>> based off an earlier version of master, then an update will be rebased on
>> the lastest master, with the original work along with any conflict
>> resolution smashed into the same unit. This always seemed a bit
>> counter-intuitive to me, because it puts information about the project
>> history that could be stored in git, using merge commits, instead inside of
>> the database of the web app.
>>
>> - Jason
>>
>> On Fri, Apr 15, 2016 at 10:08 AM, Todd Lipcon <todd@cloudera.com> wrote:
>>
>>> +1 for Gerrit from me too -- we're using it on Kudu and everyone on the
>>> team likes it.
>>>
>>> Happy to help admin the server on the gerrit.cloudera.org box which hosts
>>> Kudu and Impala
>>>
>>> -Todd
>>>
>>> On Fri, Apr 15, 2016 at 8:48 AM, Wes McKinney <wes@cloudera.com> wrote:
>>>
>>> > In my experience, GitHub pull requests are only appropriate for patches
>>> > that do not evolve significantly from the first iteration. Changes to
>>> > patches frequently cause outstanding points of discussion to be obscured
>>> > (the dreaded "comment on an outdated diff").
>>> >
>>> > Rebasing also frequently puts GitHub through a loop.
>>> >
>>> > Too often, as a result, it's tempting to rubber-stamp large patches on
>>> > GitHub PRs vs reviewing with great care (which the tool penalizes you
>>> for,
>>> > IMHO).
>>> >
>>> > Like Jacques my preference is to have a Gerrit instance available that we
>>> > can opt in to (I would also like to use Gerrit for parquet-cpp), but not
>>> > require a heavier process necessarily for small patches.
>>> >
>>> > On Friday, April 15, 2016, Zheng, Kai <kai.zheng@intel.com> wrote:
>>> >
>>> > > I'm not seeing either is good over the other, but did notice that many
>>> > > good discussions in PR reviewing not seen here, though concrete
>>> comments
>>> > > for some codes in place are very convenient comparing with JIRA based
>>> > > reviewing. No one just looks to be perfect.
>>> > >
>>> > > Regards,
>>> > > Kai
>>> > >
>>> > > -----Original Message-----
>>> > > From: Wes McKinney [mailto:wes@cloudera.com <javascript:;>]
>>> > > Sent: Friday, April 15, 2016 1:38 AM
>>> > > To: dev@arrow.apache.org <javascript:;>
>>> > > Subject: Code review tools for Arrow patches
>>> > >
>>> > > hello all,
>>> > >
>>> > > We're reaching a junction where larger patches to the Arrow codebase
>>> will
>>> > > become more frequent, and effective code reviews will be important
part
>>> > of
>>> > > maintaining a high quality project going forward.
>>> > >
>>> > > In general, the GitHub pull request UI is not generally thought of
as
>>> > very
>>> > > productive in large code reviews (some recent exposition on this
>>> > > topic:
>>> > >
>>> >
>>> http://www.beepsend.com/2016/04/05/abandoning-gitflow-github-favour-gerrit/
>>> > > ).
>>> > > Many large engineering teams prefer such (git-centric) tools as Gerrit,
>>> > > though there are other code review tools available.
>>> > >
>>> > > I don't think we are at a point where a particular code review process
>>> > > should be enforced, but more that we should have more tools available
>>> for
>>> > > groups of Arrow committers who wish to collaborate in a particular
way.
>>> > >
>>> > > As I'm familiar with Gerrit from working on Apache projects that
>>> > > Cloudera's involved with, my bias would be to try to get an instance
>>> set
>>> > up
>>> > > so that larger patches can be reviewed in a more detailed and
>>> > transactional
>>> > > way. For example: we could use gerrit.cloudera.org (like Kudu and
>>> > > Impala), but I would be happy to use any infrastructure provider.
>>> > >
>>> > > There has been some resistance / inaction within the ASF to create
an
>>> > > ASF-managed Gerrit instance:
>>> > > https://issues.apache.org/jira/browse/INFRA-2205.
>>> > >
>>> > > I'm interested to hear other perspectives.
>>> > >
>>> > > Thanks,
>>> > > Wes
>>> > >
>>> >
>>>
>>>
>>>
>>> --
>>> Todd Lipcon
>>> Software Engineer, Cloudera
>>>

Mime
View raw message