drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ted Dunning <ted.dunn...@gmail.com>
Subject Re: [DISCUSS] Allowing the option to use github pull requests in place of reviewboard
Date Wed, 24 Jun 2015 04:32:45 GMT
Travis won't likely be faster for the CI part, but it is super easy to set
up so it can't hurt to try it out.

On Tue, Jun 23, 2015 at 9:10 PM, Chris Westin <chriswestin42@gmail.com>
wrote:

> And I'll bet GitHub is a lot faster too.
>
> On Tue, Jun 23, 2015 at 2:23 PM, Hanifi Gunes <hgunes@maprtech.com> wrote:
>
> > +1
> >
> > At the very least GitHub will be UP.
> >
> > On Tue, Jun 23, 2015 at 2:18 PM, Parth Chandra <pchandra@maprtech.com>
> > wrote:
> >
> > > +1 on trying this. RB has been pretty painful to us.
> > >
> > >
> > >
> > > On Mon, Jun 22, 2015 at 9:45 PM, Matthew Burgess <mattyb149@gmail.com>
> > > wrote:
> > >
> > > > Is Travis <https://travis-ci.org/>  a viable option for the GitHub
> > > route?
> > > > I
> > > > use it for my own projects to build pull requests (with additional
> code
> > > > quality targets like CheckStyle, PMD, etc.). Perhaps that would take
> > some
> > > > of
> > > > the burden off the reviewers and let them focus on the proposed
> > > > implementations, rather than some of the more tedious aspects of each
> > > > review.
> > > >
> > > > From:  Jacques Nadeau <jacques@apache.org>
> > > > Reply-To:  <dev@drill.apache.org>
> > > > Date:  Monday, June 22, 2015 at 10:22 PM
> > > > To:  "dev@drill.apache.org" <dev@drill.apache.org>
> > > > Subject:  Re: [DISCUSS] Allowing the option to use github pull
> requests
> > > in
> > > > place of reviewboard
> > > >
> > > > I'm up for this if we deprecate the old way.  Having two different
> > > > processes seems like overkill.  In general, I find the review
> interface
> > > of
> > > > GitHub less expressive/clear but everything else is way better.
> > > >
> > > > On Mon, Jun 22, 2015 at 6:59 PM, Steven Phillips <
> > sphillips@maprtech.com
> > > >
> > > > wrote:
> > > >
> > > > >  +1
> > > > >
> > > > >  I am in favor of giving this a try.
> > > > >
> > > > >  If I remember correctly, the reason we abandoned pull requests
> > > > originally
> > > > >  was because we couldn't close the pull requests through Github.
A
> > > > solution
> > > > >  could be for whoever pushes the commit to the apache git repo to
> add
> > > the
> > > > >  Line "Closes <request number>". Github would then automatically
> > close
> > > > the
> > > > >  pull request.
> > > > >
> > > > >  On Mon, Jun 22, 2015 at 1:02 PM, Jason Altekruse <
> > > > altekrusejason@gmail.com
> > > > >>  >
> > > > >  wrote:
> > > > >
> > > > >>  > Hello Drill developers,
> > > > >>  >
> > > > >>  > I am writing this message today to propose allowing the
use of
> > > github
> > > > >  pull
> > > > >>  > requests to perform reviews in place of the apache reviewboard
> > > > instance.
> > > > >>  >
> > > > >>  > Reviewboard has caused a number of headaches in the past
few
> > > months,
> > > > and
> > > > >  I
> > > > >>  > think its time to evaluate the benefits of the apache
> > > infrastructure
> > > > >>  > relative to the actual cost of using it in practice.
> > > > >>  >
> > > > >>  > For clarity of the discussion, we cannot use the complete
> github
> > > > >  workflow.
> > > > >>  > Comitters will still need to use patch files, or check
out the
> > > branch
> > > > >  used
> > > > >>  > in the review request and push to apache master manually.
I am
> > not
> > > > >>  > advocating for using a merging strategy with git, just
for
> using
> > > the
> > > > >  github
> > > > >>  > web UI for reviews. I expect anyone generating a chain
of
> commits
> > > as
> > > > >>  > described below to use the rebasing workflow we do today.
> > > > Additionally
> > > > >  devs
> > > > >>  > should only be breaking up work to make it easier to review,
we
> > > will
> > > > not
> > > > >  be
> > > > >>  > reviewing branches that contain a bunch of useless WIP
commits.
> > > > >>  >
> > > > >>  > A few examples of problems I have experienced with reviewboard
> > > > include:
> > > > >>  > corruption of patches when they are downloaded, the web
> interface
> > > > showing
> > > > >>  > inconsistent content from the raw diff, and random rejection
of
> > > > patches
> > > > >>  > that are based directly on the head of apache master.
> > > > >>  >
> > > > >>  > These are all serious blockers for getting code reviewed
and
> > > > integrated
> > > > >>  > into the master branch in a timely manner.
> > > > >>  >
> > > > >>  > In addition to serious bugs in reviewboard, there are a
number
> of
> > > > >>  > difficulties with the combination of our typical dev workflow
> and
> > > how
> > > > >>  > reviewboard works with patches. As we are still adding
features
> > to
> > > > Drill,
> > > > >>  > we often have several weeks of work to submit in response
to a
> > JIRA
> > > > or
> > > > >>  > series of related JIRAs. Sometimes this work can be broken
up
> > into
> > > > >>  > independent reviewable units, and other times it cannot.
When a
> > > > series of
> > > > >>  > changes requires a mixture of refactoring and additions,
the
> > > process
> > > > is
> > > > >>  > currently quite painful. Ether reviewers need to look through
a
> > > giant
> > > > >  messy
> > > > >>  > diff, or the submitters need to do a lot of extra work.
This
> > > > involves not
> > > > >>  > only organizing their work into a reviewable series of
commits,
> > but
> > > > also
> > > > >>  > generating redundant squashed versions of the intermediate
work
> > to
> > > > make
> > > > >>  > reviewboard happy.
> > > > >>  >
> > > > >>  > For a relatively simple 3 part change, this involves creating
3
> > > > >  reviewboard
> > > > >>  > pages. The first will contain the first commit by itself.
The
> > > second
> > > > will
> > > > >>  > have the first commits patch as a parent patch with the
next
> > change
> > > > in
> > > > >  the
> > > > >>  > series uploaded as the core change to review. For the third
> > > change, a
> > > > >>  > squashed version of the first two commits must be generated
to
> > > serve
> > > > as a
> > > > >>  > parent patch and then the third changeset uploaded as the
> > > reviewable
> > > > >>  > change. Frequently a change to the first commit requires
> > > > regenerating all
> > > > >>  > of these patches and uploading them to the individual review
> > pages.
> > > > >>  >
> > > > >>  > This gets even worse with larger chains of commits.
> > > > >>  >
> > > > >>  > It would be great if all of our changes could be small
units of
> > > > work, but
> > > > >>  > very frequently we want to make sure we are ready to merge
a
> > > complete
> > > > >>  > feature before starting the review process. We need to
have a
> > > better
> > > > way
> > > > >  to
> > > > >>  > manage these large review units, as I do not see the
> possibility
> > of
> > > > >>  > breaking up the work into smaller units as a likely solution.
> We
> > > > still
> > > > >  have
> > > > >>  > lots of features and system cleanup to work on.
> > > > >>  >
> > > > >>  > For anyone unfamiliar, github pull requests are based on
a
> branch
> > > you
> > > > >  push
> > > > >>  > to your personal fork. They give space for a general
> discussion,
> > as
> > > > well
> > > > >  as
> > > > >>  > allow commenting inline on the diff. They give a clear
> reference
> > to
> > > > each
> > > > >>  > commit in the branch, allowing reviewers to see each piece
of
> > work
> > > > >>  > individually as well as provide a "squashed" view to see
the
> > > overall
> > > > >>  > differences.
> > > > >>  >
> > > > >>  > For the sake of keeping the project history connected to
JIRA,
> we
> > > > can see
> > > > >>  > if there is enough automatic github integration or possibly
> > upload
> > > > patch
> > > > >>  > files to JIRA each time we update a pull request. As an
side
> > note,
> > > > if we
> > > > >>  > don't need individual patches for reviewboard we could
just put
> > > patch
> > > > >  files
> > > > >>  > on JIRA that contain several commits. These are much easier
to
> > > > generate
> > > > >  an
> > > > >>  > apply than a bunch of individual files for each change.
This
> > should
> > > > >  prevent
> > > > >>  > JIRAs needing long lists of patches with names like
> > > > >>  > DRILL-3000-part1-version3.patch
> > > > >>  >
> > > > >
> > > > >
> > > > >
> > > > >  --
> > > > >   Steven Phillips
> > > > >   Software Engineer
> > > > >
> > > > >   mapr.com
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
>

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