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

[Jeffrey Altman]

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

On 4/11/2012 5:13 PM, Andrew Deason wrote:
> On Wed, 11 Apr 2012 15:42:09 -0400
> Jeffrey Altman <jaltman@your-file-system.com> wrote:
>=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=
=2E
>>>    For example, if someone asks me to review one specific change, I'l=
l
>>>    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.
>=20
> 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.
>=20
> 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?

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 tha=
t.

 * Is there a particular project that you want to see advance?  Review th=
at.

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

+1

>> 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?
>=20
> 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 :)
>=20
> 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 completely understand.

>> 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.
>=20
> 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 th=
e
> 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'r=
e
> visibly getting the commit closer to inclusion.

nod.

>> 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.
>=20
> Sure, but I'm saying this is just another demotivator for performing
> review, if what to do specifically is not clear.
>=20
> 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 expe=
rt?

The gatekeepers are always going to take into account the competence of
the reviewer.  Our trust level in a particular reviewer will always be
low at first and will increase over time as we on-going quality reviews.

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

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.

Given the rate of change in the tree with the help of Gerrit I don't
think it is even possible for someone to develop a year long out of tree
project without constantly pushing code upstream on a continuous basis.
 The Windows redirector is a perfect example.  Changes were pushed
upstream throughout the project so that in the end there was a
relatively small patchset that modified existing code when adding the
new redirector modules.  We also had sufficient reviewers with
appropriate expertise lined up to verify the submission.

For the most part I think there will be ample time if we see patches
being submitted to Gerrit that restructure the code base to ask
questions if someone has not explicitly hired recognized experts to
perform architectural reviews.  An example of doing so is when YFS hired
Jeffrey Hutzelman to perform the architectural review for thread safe
ubik.   That review was then used to guide Marc Dionne's implementation
and as a basis for validating the resulting work.

Jeffrey Altman


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

iQEcBAEBAgAGBQJPhlG1AAoJENxm1CNJffh4lfkIANyh/X2HdROWdcGnAa9N2nQp
U/fIXCdEuMkfonMM6yvhwSmEBIsJpituLgtwx1FZrhkbndhmeKUw2kXeWI9EwWmt
LugU+C1X4A9RmvtgVbrhSWT8QzPr4K4HtYIyWKSK1LSi8QZ9VYfKweDhDLNs/tqM
qYilNIFSxi/yhT9IV6at5hhxaKl0g4cvDV6ghHemZDmgrtzMOfqZ33aynYQwrvwq
5szz8XCVLCRAIEBFXk9GXis1L+IT31egsmHJrhmSyyhr8mg14UhFu3t23bsF9vjc
893XF6of9H9Wqjaw5sTh9uSIwB9V3TZ6iiTMxLD3pQvzPFu3Uy+EsNa25gG2JaM=
=uVf9
-----END PGP SIGNATURE-----

--------------enig46CDE064920529F7CDBD0B76--