Open Stack

On Tue, 2015-04-07 at 18:12 +0000, Tim Bell wrote:
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley at HansenPartnership.com]
> > Sent: 07 April 2015 19:03
> > To: Michael Still
> > Cc: OpenStack Development Mailing List (not for usage questions)
> > Subject: [openstack-dev] Fixing the Nova Core Reviewer Frustration [was Re:
> > [Nova] PTL Candidacy]
> > 
> > On Tue, 2015-04-07 at 11:27 +1000, Michael Still wrote:
> > > Additionally, we have consistently asked for non-cores to help cover
> > > the review load. It doesn't have to be a core that notices a problem
> > > with a patch -- anyone can do that. There are many people who do help
> > > out with non-core reviews, and I am thankful for all of them. However,
> > > I keep meeting people who complain about review delays, but who don't
> > > have a history of reviewing themselves. That's confusing and
> > > frustrating to me.
> > 
> > I can understand why you're frustrated, but not why you're surprised:
> > the process needs to be different.  Right now the statement is that for a patch
> > series to be accepted it has to have a positive review from a core plus one other,
> > however the "one other" can be a colleague, so it's easy.  The problem, as far as
> > submitters see it, is getting that Core Reviewer.  That's why so much frenzy
> > (which contributes to your
> > frustration) goes into it.  And why all the complaining which annoys you.
> > 
> > To fix the frustration, you need to fix the process:  Make the cores more of a
> > second level approver rather than a front line reviewer and I predict the frenzy
> > to get a core will go down and so will core frustration.  Why not require a +1
> > from one (or even more than one) independent (for some useful value of
> > independent) reviewer before the cores will even look at it?  That way the cores
> > know someone already thought the patch was good, so they're no longer being
> > pestered to review any old thing and the first job of a submitter becomes to find
> > an independent reviewer rather than go bother a core.
> > 
> 
> If I take a case that we were very interest in
> (https://review.openstack.org/#/c/129420/) for nested project quota,
> we seemed to need two +2s from core reviewers on the spec. 
> 
> There were many +1s but these did not seem to result in an increase in
> attention to get the +2s. Initial submission of the spec was in
> October but we did not get approval till the end of January.
> 
> Unfortunately, we were unable to get the code into the right shape
> after the spec approval to make it into Kilo.
>
> One of the issues for the academic/research sector is that there is a
> significant resource available from project resources but these are
> time limited. Thus, if a blueprint and code commit cannot be completed
> within the window for the project, the project ends and resources to
> complete are no longer available. Naturally, rejections on quality
> grounds such as code issues or lack of test cases is completely
> reasonable but the latency time can extend the time to delivery
> significantly.
> 
> Luckily, in this case, the people concerned are happy to continue to
> completion (and the foundation is sponsoring the travel for the summit
> too) but this would not always be the case.

So I think you're telling me that my proposed process would elongate the
submission window and this would cause more stuff to drop because of the
fixed timeframes people have to get stuff in? I think I have to admit to
that, but would like to propose some refinements based on the analysis
below.

There were 35 patch sets in this review spanning a time period from 18
October to 21 Jan, when it was included.  Up to patch set 17, the
non-core reviews were mostly -1.  From 17-27 there was some back and
forth satisfying various reviewer comments and updating the blueprint.
And from 28-34 the comments were getting more positive.  35 was the true
OK point (except that gerrit kicked it out for a merge problem) and 36
was the accepted code.

Under the process I proposed, the cores wouldn't have got involved until
somewhere around patch set 18-20; I don't think this would cause
anything to lengthen (patch set 20 was Dec 13).  However, looking at the
history, the refinements I would suggest are no -1s to pass to the
cores, so they wouldn't actually get involved until patch set 26.

If I look at the history, I also see some reviewers dropping out once
their concerns and review comments have been addressed (after giving a
+1), so the other thing I'd suggest is that instead of erasing the
review history on each patch submission, it carries over (at least the
-1 and +1) so you don't have to wait a while for a consensus to form
(reviewers would, of course, be able to alter their votes at any time).
The pressure is thus on the submitter to make the changes to switch
every original -1 to a +1.

James