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

[Jeffrey Altman]

This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enig6BB0C48F0A3455C007C92FAE
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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


>> 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?
>=20
> Maybe. If nothing else, I think this would help make it much more clear=

> why some things are not getting merged...
>=20
> There are a few reasons why I don't review things more, which may or ma=
y
> not be shared by others:
>=20
>  - 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.
 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?

>  - 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 day=
s
>    are very busy. Review requirements like you suggest could improve
>    this; they have the obvious downside of making changes go in more
>    slowly (potentially).

We try to keep things flowing.  Derrick is especially conscious of the
fact that large numbers of unreviewed patchsets become intimidating.
Therefore, he is aggressive about pushing changes that buildbot
verifies, which make sense, are easy to test, and are likely to be
non-controversial.

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 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 no=
t
>    bad; more review is more review regardless.

I think more review is a good thing.  I also think that by requiring
more votes in favor that it would provide an indication to the
gatekeepers of where their efforts are best spent.   Still, a support
provider such as SNA or a product development company such as YFS which
has multiple active OpenAFS contributors on staff would have a distinct
edge.  That edge of course would not be new.

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

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.

>    I don't know if that can/should be fixed for not, but it is a source=

>    of anxiety for me nonetheless.
> =20
>  - I had a conversation with someone where the issue was brought up tha=
t
>    gerrit reviewers these days often only object to something based on
>    little things (whitespace, typo, obvious small bugs). More general o=
r
>    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:
>=20
>   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.
>=20
>    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.

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.

You can also raise the subject privately with openafs-gatekeepers if you
want feedback on your review before you post it.

Applying a -1 to a patchset requesting that it not be approved until you
have had a chance to perform a more thorough review is also acceptable.

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

> 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.
>=20
> 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.

Being a core developer in a widely used open source project requires a
tough skin.   You are doing a good job.  Keep it up.

Jeffrey Altman



--------------enig6BB0C48F0A3455C007C92FAE
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)

iQEcBAEBAgAGBQJPhd6WAAoJENxm1CNJffh4SAIIAJfP8qjUeFVNLu4wwYCfQNwT
eXB9L4nILRCwAk/5Ei9sb+agjiNwt06+C4vQ0jtmlPT3DqwFAAh5FQqOIeVxhwUk
J4dMWy1LmvOYrI3NQQsBolxdjredrhHcbfnyzlUesub4lgFls8ka+o0aRf/6T8/A
Jp+mdjSyU9at9E9ETThEAnE9vNySAtXciIM8eXIh5fnqN6ne/+dabV8Lz8EIKiCl
IAVmd3UukQ+95Zv8g/m/tjYf6nMD8qUt8NZK610kaA3sAJICgqMnDzxGfCmuUi9f
InupJOzl0uVazhI3Q8Ij3xGKkD607UboRVN5ungpVpSxEaRIm+yifFXIyrpleYQ=
=6b/q
-----END PGP SIGNATURE-----

--------------enig6BB0C48F0A3455C007C92FAE--