[OpenAFS-devel] Re: Gerrit reviews and the rate of acceptance

[Andrew Deason]

On Wed, 11 Apr 2012 15:42:09 -0400
Jeffrey Altman <jaltman@your-file-system.com> wrote:

> >  - There are a ton of changes all the time, which is overwhelming. If
> >    there were a subset that I were asked to focus on or prioritize, I
> >    would be much more likely to review those in a more timely fashion.
> >    For example, if someone asks me to review one specific change, I'll
> >    get to it. But when there's, say, 200, even just doing one seems a
> >    much more daunting task.
> 
> if this is daunting for you, and you work on the code every day as a
> paid support engineer, imagine how it feels for others.   I don't know
> how we can change this.  Imposing an additional task on the
> gatekeepers by requiring that they invite reviewers seems to be to be
> a non-starter.

I wasn't suggesting that, but I wasn't really suggesting any other
solution, either. I'm just saying, when I see a big pile of unorganized,
unprioritized, endless work, it's less appealing than the other very
neat and organized endless piles of work that I have.

So, I'm just saying, that's a demotivator for performing review. I don't
know if there are any guidelines to provide for people or some way for
them to determine what specifically to look at. As a random person
looking to review stuff should I try looking at... the oldest changes?
The newest ones? The ones with the least amount of review? The smallest
ones? The largest ones? The one with the lowest IP address? The changes
submitted by the most physically attractive developer?

Now, from the other side of things, if anyone needs to solicit reviews,
I would've put that onus on the submitter (or any interested 3rd
parties) before the gatekeepers. If you want something to get in, you
should go bother people to review the change. (For people that aren't
regulars in the community, point them at -devel, jabber, etc to solicit)
If I submitted something I really want in the tree, I certainly do not
mind going around bothering people until they review it.

> If a full time support engineer that is paid to work on OpenAFS cannot
> find the time to review Gerrit every day, why should anyone else be
> able to?

Well, for one thing, like you said elsewhere, I am not paid to review
patches in gerrit (nor is anyone else, to my knowledge) except in very
specific circumstances. (nearly) all of my (scarce) review is done
during my spare time, so it typically happens when I'm avoiding other
work. So technically, if I weren't a paid support engineer and did
nothing all day, I would find this much less daunting :)

However, speaking as an employee of SNA, I could try to get some amount
of weekly block of hours dedicated to reviewing nonspecific gerrit
patchsets or something. But that's a conversation to be had internally,
and involves factors that are out my control, so I cannot provide
guarantees.

> I do believe that requiring a +4 or +5 would slow things down but it
> could also potentially speed things up by getting more people into the
> habit of performing reviews.  It would especially help if more people
> had time performing reviews built into their job description.

It also may help by providing a very concrete, very visible value in
doing them. I mean, right now if you give a +1, you get your name on the
commit and the little +1 graphic appears. "who cares". But if you're
actually getting the total "score" closer to what it needs to be, you're
visibly getting the commit closer to inclusion.

> When I have performed a brief glance and have not found anything
> wrong, I leave a review and vote +0.   Even a +1 vote can be augmented
> to indicate in writing what it is that you did during the review.
> Everyone does not have to review the same way provided that your
> intention when performing the review is communicated.

Sure, but I'm saying this is just another demotivator for performing
review, if what to do specifically is not clear.

I dunno, I may be alone, but I think thise goes back to what Jason said
about being kinda unsure about the required qualification level for
reviews. It's just a general sense of... unsure-ness about what to do.
If the score itself doesn't really matter so much as what you said, and
if potential reviewers shouldn't worry about qualifications, then...
that shouldn't be just left implicit, it should be said. I just think
it's more encouraging.

> I think it is perfectly fine for you to voice architectural design
> comments as part of a review.  Especially if the design was not
> discussed on openafs-devel before hand.

Well, that's your opinion and I think it's a good one, but it doesn't
change it that objecting to things like that is still a really bad
situation to arise. It doesn't encourage... happy friendly development
practices. I think encouraging (or even requiring in some distant
future) people to not get in that situation in the first place is very
important.

> >  - Viewing or making inline comments in files is _still_ broken in
> >    Opera, which I find immensely frustrating (gerrit issue 878). I
> >    don't really know what to do about this, since debugging GWT
> >    compiled javascript (or whatever it is) is a bit beyond me. (I
> >    really wish I could just give reviews via email...)
> 
> Its also broken in the webkit used on iOS and in Internet Explorer.  I
> don't think there is anything we can do about it other than remember
> to use Firefox or Chrome when performing OpenAFS patch reviews.

Augh. 

Well, I think that's at least some way that someone without an OpenAFS
skillset could help. If someone knows a lot about GWT or knows anyone
they can bother about it...

-- 
Andrew Deason
adeason@sinenomine.net