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

[Andrew Deason]

On Wed, 11 Apr 2012 23:53:20 -0400
Jeffrey Altman <jaltman@your-file-system.com> wrote:

> I would say that you should prioritize the work based whatever
> criteria matters most to you.
> 
>  * Do you want to learn a new section of code?   Review that.
> 
>  * Do you want to review something you are already expert at?  Review that.
> 
>  * Is there a particular project that you want to see advance?  Review that.

Yes, but what if none of these are true for a particular time? (at
least, as far as what is in gerrit awaiting review) What if, as a random
reviewer, my goal is "I want to help the project" or "I want Jeff to
stop complaining" :) What does the _project_ want, if all other things
are equal?

(Not to mention that basing it off of what the change actually does can
be a little more difficult, since you may need to look inside each
changeset to make that determination.)

> > I dunno, I may be alone, but I think this 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.
> 
> It really is a bit of a catch-22.  If only experts in the code base
> are permitted to be reviewers, how is anyone new ever going to become
> an expert?

That sounds like the complete opposite of what I was trying to say. If
we want to encourage less-experienced people to engage in review (which
I thought we did / what you were saying), we probably need to make it
explicitly clear that we welcome them. Most people are not going to be
confident in their understanding of something for performing a review
(since imo most of the code has very few people in the world that fully
understand it), and, except for some people that have been around for
awhile, I think it's very non-obvious that that's okay.

> This isn't a new problem.  Its the tug-of-war the gatekeepers have
> faced since the beginning of the project.  Its why when Gerrit was
> deployed that we stated that we would never again accept a large
> disruptive contribution into the tree.

I know it isn't new; it has been a problem for as long as I've been
around, and from what I hear, has probably been an issue for the entire
lifetime of the project.

The only time I am ever aware of that people do pre-coding design/arch
review is during those hackathons, or in closed backchannel discussions.
This isn't how it has to be, and I think it would really help if people
quit doing it like that.

-- 
Andrew Deason
adeason@sinenomine.net