Open Stack

On 9/2/15 5:19 AM, Gorka Eguileor wrote:
> On Tue, Sep 01, 2015 at 09:30:26AM -0600, John Griffith wrote:
>> On Tue, Sep 1, 2015 at 5:57 AM, Tom Barron <tpb at dyncloud.net> wrote:
>>
>>> [Yesterday while discussing the following issue on IRC, jgriffith
>>> suggested that I post to the dev list in preparation for a discussion in
>>> Wednesday's cinder meeting.]
>>>
>>> Please take a look at the 10 "Low" priority reviews in the cinder
>>> Liberty 3 etherpad that were punted to Mitaka yesterday. [1]
>>>
>>> Six of these *never* [2] received a vote from a core reviewer. With the
>>> exception of the first in the list, which has 35 patch sets, none of the
>>> others received a vote before Friday, August 28.  Of these, none had
>>> more than -1s on minor issues, and these have been remedied.
>>>
>>> Review https://review.openstack.org/#/c/213855 "Implement
>>> manage/unmanage snapshot in Pure drivers" is a great example:
>>>
>>>    * approved blueprint for a valuable feature
>>>    * pristine code
>>>    * passes CI and Jenkins (and by the deadline)
>>>    * never reviewed
>>>
>>> We have 11 core reviewers, all of whom were very busy doing reviews
>>> during L3, but evidently this set of reviews didn't really have much
>>> chance of making it.  This looks like a classic case where the
>>> individually rational priority decisions of each core reviewer
>>> collectively resulted in starving the Low Priority review queue.
>>>
> 
> I can't speak for other cores, but in my case reviewing was mostly not
> based on my own priorities, I reviewed patches based on the already set
> priority of each patch as well as patches that I was already
> reviewing.
> 
> Some of those medium priority patches took me a lot of time to review,
> since they were not trivial (some needed some serious rework).  As for
> patches I was already reviewing, as you can imagine it wouldn't be fair
> to just ignore a patch that I've been reviewing for some time just when
> it's almost ready and the deadline is closing in.
> 

That's why I said that this situation is an outcome of individually
rational decisions.  It should be clear that none of this is intended as
a complaint about reviewers or reviewer's performance.

> Having said that I have to agree that those patches didn't have much
> chances, and I apologize for my part on that.  While it is no excuse I
> have to agree with jgriffith when he says that those patches should have
> pushed cores for reviews (even if this is clearly not the "right" way to
> manage it).

No apology required!

> 
>>> One way to remedy would be for the 11 core reviewers to devote a day or
>>> two to cleaning up this backlog of 10 outstanding reviews rather than
>>> punting all of them out to Mitaka.
>>>
>>> Thanks for your time and consideration.
>>>
>>> Respectfully,
>>>
>>> -- Tom Barron
>>>
>>> [1] https://etherpad.openstack.org/p/cinder-liberty-3-reviews
>>> [2] At the risk of stating the obvious, in this count I ignore purely
>>> procedural votes such as the final -2.
>>>
>>> __________________________________________________________________________
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>
>> ​Thanks Tom, this is sadly an ongoing problem every release.  I think we
>> have a number of things we can talk about at the summit to try and
>> make some of this better.  I honestly think that if people were to
>> actually "use" launchpad instead of creating tracking etherpads
>> everywhere it would help.  What I mean is that there is a ranked
>> targeting of items in Launchpad and we should use it, core team
>> members should know that as the source of truth and things that must
>> get reviewed.
>>
> 
> I agree, we should use Launchpad's functionality to track BPs and Bugs
> targeted for each milestone, and maybe we can discuss on a workflow that
> helps us reduce starvation at the same time that helps us keep track of
> code reviewers responsible for each item.
> 
> Just spitballing here, but we could add to BP's work items and Bug's
> comments what core members will be responsible for reviewing related
> patches.  Although this means cores will have to check this on every
> review they do that has a BP or Bug number, so if there are already 2
> cores responsible for that feature they should preferably just move on
> to other patches and if there are not 2 core reviewers they should add
> themselves to LP.  This way patch owners know who they should bug for
> reviews on their patches and if there are no core reviewers for them
> they should look for some (or wait for them to "claim" that
> bug/feature).
> 
>> As far as Liberty and your patches; Yesterday was the freeze point, the
>> entire Cinder team agreed on that (yourself included both at the mid-cycle
>> meet up and at the team meeting two weeks ago when Thingee reiterated the
>> deadlines).  If you noticed last week that your patches weren't going
>> anywhere YOU should've wrangled up some reviews.
>>
>> Furthermore, I've explained every release for the last 3 or 4 years that
>> there's no silver bullet, no magic process when it come to review
>> throughput.  ESPECIALLY when it comes to the 3'rd milestone.  You can try
>> landing strips, priority listed etherpads, sponsors etc etc but the fact is
>> that things happen, the gate slows down (or we completely break on the
>> Cinder side like we did yesterday).  This doesn't mean "oh, well then you
>> get another day or two", it means stuff happens and it sucks but first
>> course of action is drop low priority items.  It just means if you really
>> wanted it you probably should've made it happen earlier.  Just so you know,
>> I run into this every release as well.  I had a number of things in
>> progress that I had hoped to finish last week and yesterday, BUT my
>> priority shifted to trying to help get the cinder patches back on track and
>> get the items in Launchpad updated to actually reflect something that was
>> somewhat possible.
>>
>> The only thing that works is "submit early and review often" it's simple.
>>
> 
> While this is true, sometimes it's not a matter of submitting early,
> because some patches just keep getting ignored and when deadline closes
> in low priority patches will always have higher priority patches that
> will go before them, unless we set a workflow that will give them a
> better chance of getting reviews.
> 
>> Finally, I pointed out to you yesterday that we could certainly discuss as
>> a team what to do with your patches.  BUT that given how terribly far
>> behind we were in the process that I wanted reviewers to focus on medium,
>> high and critical prioritized items.  That's what prioritization's are for,
>> it means when crunch time hits and things hit the fan it's usually the
>> "low" priority things that get bumped.
>>
>> Thanks,
> 
> I am not against discussing this, but in all fairness we should discuss
> *everybody's* patches that were targeted for L3, not just tbarron's.

Completely agree, and I hope that everyone understands that this is the
way I've been trying to frame the issue myself.

-- Tom
> 
> Cheers,
> Gorka.
>