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 <t...@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 <w...@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.zh...@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:w...@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 >
