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

[Andrew Deason]

On Wed, 11 Apr 2012 13:00:08 -0400
Jeffrey Altman <jaltman@your-file-system.com> wrote:

> The first theme is that it takes too long for patches to be accepted
> into OpenAFS.  As evidence of the overwhelming delays is the nearly
> 100 patchsets that are sitting in Gerrit for more than two weeks.
> Many of which have been sitting there for nearly two years.

While I don't doubt that some people are really annoyed by this, this
isn't really the case for me. If someone really cares about / needs
something merged, in my experience if you make enough noise about it
someone will do something about it.

That is, the only submissions of mine that sit there ~forever are the
ones that I just don't really care that much about.

> If we were to impose a requirement that every patchset must receive a
> total vote of +4 or +5 before it could be submitted, would that help
> to obtain reviews?

Maybe. If nothing else, I think this would help make it much more clear
why some things are not getting merged...

There are a few reasons why I don't review things more, which may or may
not be shared by others:

 - 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.
 
 - The rate at which changes are merged is sometimes faster than I have
   time to even look at gerrit. If something gets merged 2 days after
   its submitted... well, I can't look at it every single day; some days
   are very busy. Review requirements like you suggest could improve
   this; they have the obvious downside of making changes go in more
   slowly (potentially).
   
   It also may create an arguably unfair difference in the barrier to
   entry in getting submissions merged. A random person submitting
   something may have to work to get 3 or 4 others to look at a
   submission, but I can probably get Mike Meffie, Tom Keiser, etc to
   look at something fairly easily (and vice versa). Or maybe that's not
   bad; more review is more review regardless.
 
 - I have always been uncomfortable with the limited scores
   non-gatekeepers can give. There is no difference in the scoring
   system between "I briefly glanced at this and I didn't happen to see
   anything wrong" and "I have reviewed this thoroughly and believe it
   to be correct". And those two are very different for me... with the
   former I'm just kind of checking for obvious or common mistakes, and
   with the second I'm actually checking the flow/logic and seeing if
   the change does what it's supposed to. The former I can do for a lot
   of changes pretty quickly, but I feel like _someone_ should do the
   latter at least once for a changeset.

   I don't know if that can/should be fixed for not, but it is a source
   of anxiety for me nonetheless.
 
 - I had a conversation with someone where the issue was brought up that
   gerrit reviewers these days often only object to something based on
   little things (whitespace, typo, obvious small bugs). More general or
   architectural review is not done there, as it may not really be an
   appropriate place for it, and arch review is done... nowhere
   (public). I am not sure how to tell someone "this is the completely
   wrong approach and you really should just redo the whole thing"
   without getting a response involving one or more of:

  1. The submitter getting pissed off at me.
  2. The submitter just giving up and going away.
  3. The submitter not realizing/understanding that what I'm saying is
     just an opinion and not some kind of official position of... well,
     anything/anyone.

   I think this would be helped by having some kind of environment that
   is more welcoming of review in the design/architecture stages of
   something, before code is submitted to gerrit. Currently it is
   often impossible to object to something on an architectural level
   until someone has already implemented the whole thing.

 - 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...)

That may not be everything, but it's just what came to mind. I don't
know if solutions to those are feasible or necessary, but... there they
are.

I also sometimes get the feeling that people are just annoyed when I
point out some flaws, which makes me want to just keep my mouth shut...
but I probably just need to get over that.

-- 
Andrew Deason
adeason@sinenomine.net