Open Stack

On 12/07/2014 06:02 PM, Jay Pipes wrote:
> On 12/07/2014 04:19 AM, Michael Still wrote:
>> On Sun, Dec 7, 2014 at 7:03 PM, Gary Kotton <gkotton at vmware.com> wrote:
>>> On 12/6/14, 7:42 PM, "Jay Pipes" <jaypipes at gmail.com> wrote:
>>
>> [snip]
>>
>>>> -1 on pixelbeat, since he's been active in reviews on
>>>> various things AFAICT in the last 60-90 days and seems to be still a
>>>> considerate reviewer in various areas.
>>>
>>> I agree -1 for Padraig
>>
>> I'm going to be honest and say I'm confused here.
>>
>> We've always said we expect cores to maintain an average of two
>> reviews per day. That's not new, nor a rule created by me. Padraig is
>> a great guy, but has been working on other things -- he's done 60
>> reviews in the last 60 days -- which is about half of what we expect
>> from a core.
>>
>> Are we talking about removing the two reviews a day requirement? If
>> so, how do we balance that with the widespread complaints that core
>> isn't keeping up with its workload? We could add more people to core,
>> but there is also a maximum practical size to the group if we're going
>> to keep everyone on the same page, especially when the less active
>> cores don't generally turn up to our IRC meetings and are therefore
>> more "expensive" to keep up to date.
>>
>> How can we say we are doing our best to keep up with the incoming
>> review workload if all reviewers aren't doing at least the minimum
>> level of reviews?
> 
> Personally, I care more about the quality of reviews than the quantity.
> That said, I understand that we have a small number of core reviewers
> relative to the number of open reviews in Nova (~650-700 open reviews
> most days) and agree with Dan Smith that 2 reviews per day doesn't sound
> like too much of a hurdle for core reviewers.
> 
> The reason I think it's important to keep Padraig as a core is that he
> has done considerate, thoughtful code reviews, albeit in a smaller
> quantity. By saying we only look at the number of reviews in our
> estimation of keeping contributors on the core team, we are
> incentivizing the wrong behaviour, IMO. We should be pushing that the
> thought that goes into reviews is more important than the sheer number
> of reviews.
> 
> Is it critical that we get more eyeballs reviewing code? Yes, absolutely
> it is. Is it critical that we get more reviews from core reviewers as
> well as non-core reviewers. Yes, absolutely.
> 
> Bottom line, we need to balance between quality and quantity, and
> kicking out a core reviewer who has quality code reviews because they
> don't have that many of them sends the wrong message, IMO.
> 

I could not *possibly* agree more with everything Jay wrote above!

Quality should always win! And 2 reviews a day is a nice approximation
of what is expected but we should not have any number as a hard
requirement. It's lazy (in addition to sending the wrong message) and we
_need_ to be better than that!

Slightly off-topic - since we're so into numbers - Russell's statistics
were at one point showing the ratio between reviews given and reviews
received. I tend to be wary of people reviewing without writing any code
themselves, as they tend to lose touch with the actual constraints the
code is written under in different parts of Nova. This is especially
important when reviewing larger feature branches or more complicated
refactoring (a big part of what we want to prioritize in Kilo).

As any number - that one is also never going to tell the whole story,
and should not ever become a hard rule - but I for one would be
interested to see it.

N.