Just for the record, Apex had some issues getting Gerrit reviews reflected in a coherent fashion into the Apache record. I presume that you guys will have that handled or will check with the Apex devs to learn their resolution.
On Sun, Apr 24, 2016 at 5:30 PM, Wes McKinney <w...@cloudera.com> wrote: > 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 <w...@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 > > <altekruseja...@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 <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 > >>> >
