atom feed20 messages in org.apache.incubator.cloudstack-devRe: [DISCUSS][INFRA] Setting up gerrit
FromSent OnAttachments
Alex HuangFeb 8, 2013 11:06 am 
Chip ChildersFeb 8, 2013 11:09 am 
Anthony XuFeb 8, 2013 11:11 am 
David NalleyFeb 8, 2013 11:51 am 
Rohit YadavFeb 8, 2013 8:23 pm 
Hugo TrippaersFeb 9, 2013 9:32 am 
Pranav SaxenaFeb 9, 2013 11:48 am 
John KinsellaFeb 9, 2013 2:31 pm 
Alex KarasuluFeb 10, 2013 3:55 am 
Joe BrockmeierFeb 11, 2013 8:04 am 
Rohit YadavFeb 11, 2013 8:58 am 
Chip ChildersFeb 11, 2013 8:50 pm 
Leif GruenwoldtFeb 13, 2013 12:06 pm 
Prasanna SanthanamFeb 14, 2013 1:43 am 
Chiradeep VittalFeb 14, 2013 2:07 pm 
Prasanna SanthanamFeb 14, 2013 9:19 pm 
Ahmad EmneinaFeb 14, 2013 9:27 pm 
Chip ChildersFeb 19, 2013 8:20 am 
Dave CahillFeb 21, 2013 10:35 pm 
Rohit YadavFeb 21, 2013 10:39 pm 
Subject:Re: [DISCUSS][INFRA] Setting up gerrit
From:David Nalley (
Date:Feb 8, 2013 11:51:46 am

On Fri, Feb 8, 2013 at 2:06 PM, Alex Huang <> wrote:

Hi everyone,

I like to propose that we setup gerrit as the review mechanism. Here are my

- Committer status in Apache is a reflection of one's commitment to the
community, not a reflection of understanding of code. So to me just because you
have committer status shouldn't mean code does not need review. Chip's been
doing a great job monitoring the merges and commits but one person handling all
that just means things will slip through. - This also has the side effect of contributors' code contribution to be treated
as a second class citizen with delays in reviews because review is not common
place within our community. - Direct commits have to be reverted if there are -1 votes, directly impacting
the time and effort of the code contributor.

It makes a lot of sense to make code commit and review a normal process in the
CloudStack Community.

So while I love Gerrit, and would love to see it implemented, most of what you've described above are social/cultural problems, not technical ones, and thus a technical solution is unlikely to fix those things. Gerrit also requires that there actually be tests in place to test, othewise it's just another hoop to jump through. With a whopping 3% of classes covered, it doesn't really mean much at this point, unless we spend significant amounts of time building tests.

Speaking from a technical perspective - there is no ability to stop a committer from committing to the repo. And yes, committer status is a reflection of ones commitment to the project, and also of the trust we have in a person. That doesn't mean a committer is perfect.

This is also a _dramatic_ change in culture - moving from a commit then review to a review then commit - this will dramatically slow down project development. I'll note that we have past guidance from our mentors that encouraged CTR as opposed to RTC.

So let me toss out an example (and I am assuming you'll be requiring two committers/reviews to approve changes, because if we are just talking about publishing changes based on automated test infra, that's not much improvement over our current method due to our low test coverage ):

If Hugo makes some packaging changes (he's made ~30 distinct commits around packaging within the past week - on two different branches (so 60 total commits)) he then needs to get two reviews for each of the those commits - which means he either has to work in isolation on his own to land massive changes that get reviewed by two people, or he has to get 120 reviews for his changes. Then lets talk about who is qualified to really review those commits. That's a dramatic subset of committers. And then I'd ask - what has the 120 reviews given us? Massive improvement?

So in short, what we are really talking about is dramatically ramping up the number of reviews required to get something into ACS, and we've already demonstrated that we aren't capable of timely getting things reviewed in reviewboard, what makes us think that dramatically increasing the number of reviews needed to see progress is a good thing?

-1 from me, though I am open to being persuaded, because I really do like Gerrit.