Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Steven Hardy
On Mon, Sep 08, 2014 at 03:14:24PM +1200, Robert Collins wrote:
 I hope the subject got your attention :).
 
 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.
 
 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)

I think this may be a sensible move, but only if it's used primarily to
land the less complex/risky patches more quickly.

As has been mentioned already by Angus, +1 can (and IMO should) be used for
any less trival and/or risky patches, as the more-eyeballs thing is really
important for big or complex patches (we are all fallible, and -core folks
quite regularly either disagree, spot different types of issue, or just
have better familiarity with some parts of the codebase than others).

FWIW, every single week in the Heat queue, disagreements between -core
reviewers result in issues getting fixed before merge, which would result
in more bugs if the 1x+2 scheme was used unconditionally.  I'm sure other
projects are the same, but I guess this risk can be mitigated with reviewer
+1 discretion.

Steve

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Russell Bryant
On 09/08/2014 05:17 AM, Steven Hardy wrote:
 On Mon, Sep 08, 2014 at 03:14:24PM +1200, Robert Collins wrote:
 I hope the subject got your attention :).

 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.

 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)
 
 I think this may be a sensible move, but only if it's used primarily to
 land the less complex/risky patches more quickly.
 
 As has been mentioned already by Angus, +1 can (and IMO should) be used for
 any less trival and/or risky patches, as the more-eyeballs thing is really
 important for big or complex patches (we are all fallible, and -core folks
 quite regularly either disagree, spot different types of issue, or just
 have better familiarity with some parts of the codebase than others).
 
 FWIW, every single week in the Heat queue, disagreements between -core
 reviewers result in issues getting fixed before merge, which would result
 in more bugs if the 1x+2 scheme was used unconditionally.  I'm sure other
 projects are the same, but I guess this risk can be mitigated with reviewer
 +1 discretion.

Agreed with this.  I think this is a worthwhile move for simpler
patches.  I've already done it plenty of times for a very small category
of things (like translations updates).  It would be worth having someone
write up a proposal that reflects this, with some examples that
demonstrate patches that really need the second review vs others that
don't.  In the end, it has to be based on trust in a -core team member
judgement call.

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Sean Dague
On 09/08/2014 08:52 AM, Russell Bryant wrote:
 On 09/08/2014 05:17 AM, Steven Hardy wrote:
 On Mon, Sep 08, 2014 at 03:14:24PM +1200, Robert Collins wrote:
 I hope the subject got your attention :).

 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.

 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)

 I think this may be a sensible move, but only if it's used primarily to
 land the less complex/risky patches more quickly.

 As has been mentioned already by Angus, +1 can (and IMO should) be used for
 any less trival and/or risky patches, as the more-eyeballs thing is really
 important for big or complex patches (we are all fallible, and -core folks
 quite regularly either disagree, spot different types of issue, or just
 have better familiarity with some parts of the codebase than others).

 FWIW, every single week in the Heat queue, disagreements between -core
 reviewers result in issues getting fixed before merge, which would result
 in more bugs if the 1x+2 scheme was used unconditionally.  I'm sure other
 projects are the same, but I guess this risk can be mitigated with reviewer
 +1 discretion.
 
 Agreed with this.  I think this is a worthwhile move for simpler
 patches.  I've already done it plenty of times for a very small category
 of things (like translations updates).  It would be worth having someone
 write up a proposal that reflects this, with some examples that
 demonstrate patches that really need the second review vs others that
 don't.  In the end, it has to be based on trust in a -core team member
 judgement call.

One of the review queries I've got is the existing Nova patches which
already have 1 +2 on those. I'd say ~ 25% of them I find an issue in.
I'm assuming another 25% of them had an issue I didn't find.

2 +2 has been part of OpenStack culture for a long time, and there is a
good reason for it, it really does keep bugs out.

It should also be clear that the subject of this email really should
have been merging code faster, because nothing in here doubles the
review bandwidth, it just provides us with less review coverage.

I'm currently less convinced that raw core merge speed is our primary
issue right now. I think it's a symptom of accumulated debt, and
complexity growth from the features over the last couple of cycles.
Treating symptoms means we'll just be discussing this in another 6
months (if we're lucky) in a new process change thread.

-Sean

-- 
Sean Dague
http://dague.net

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Flavio Percoco
On 09/08/2014 02:52 PM, Russell Bryant wrote:
 On 09/08/2014 05:17 AM, Steven Hardy wrote:
 On Mon, Sep 08, 2014 at 03:14:24PM +1200, Robert Collins wrote:
 I hope the subject got your attention :).

 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.

 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)

 I think this may be a sensible move, but only if it's used primarily to
 land the less complex/risky patches more quickly.

 As has been mentioned already by Angus, +1 can (and IMO should) be used for
 any less trival and/or risky patches, as the more-eyeballs thing is really
 important for big or complex patches (we are all fallible, and -core folks
 quite regularly either disagree, spot different types of issue, or just
 have better familiarity with some parts of the codebase than others).

 FWIW, every single week in the Heat queue, disagreements between -core
 reviewers result in issues getting fixed before merge, which would result
 in more bugs if the 1x+2 scheme was used unconditionally.  I'm sure other
 projects are the same, but I guess this risk can be mitigated with reviewer
 +1 discretion.
 
 Agreed with this.  I think this is a worthwhile move for simpler
 patches.  I've already done it plenty of times for a very small category
 of things (like translations updates).  It would be worth having someone
 write up a proposal that reflects this, with some examples that
 demonstrate patches that really need the second review vs others that
 don't.  In the end, it has to be based on trust in a -core team member
 judgement call.
 

What about simply leaving it as-is and allow people to ninja-approve
things when they feel like it? When in doubt, reviewers should just
stick to the 2x+2 and don't approve the patch.

I'm basically saying the same thing that has been proposed but based on
the current default + an exception to the rule. What changes is the way
people approach reviews. Leaving 2x+2 as the default but allowing
exceptions where people can 1x+2A is better than making the default 1x+2
and asking people to not approve patches if they think it requires more
reviews.

Just my $0.02, I agree with the proposal of trusting cores and letting
them do 1x+2A when they think it's worth it.

Flavio

-- 
@flaper87
Flavio Percoco

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Alexis Lee
Sean Dague said on Mon, Sep 08, 2014 at 09:22:56AM -0400:
  On 09/08/2014 05:17 AM, Steven Hardy wrote:
  I think this may be a sensible move, but only if it's used primarily to
  land the less complex/risky patches more quickly.
 
 2 +2 has been part of OpenStack culture for a long time, and there is a
 good reason for it, it really does keep bugs out.
 
 It should also be clear that the subject of this email really should
 have been merging code faster, because nothing in here doubles the
 review bandwidth, it just provides us with less review coverage.

For these reasons, I'm also wary of changing this in general.

Sometimes I yell in IRC if I +2 something important, this shortens loop
time as hopefully I already understand the patch and can answer
questions.

Single-approvals for small + simple changes could be worth trying.
Perhaps also for large + simple changes like whitespace fixes.


Alexis
-- 
Nova Engineer, HP Cloud.  AKA lealexis, lxsli.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Sean Dague
On 09/08/2014 11:07 AM, Alexis Lee wrote:
 Sean Dague said on Mon, Sep 08, 2014 at 09:22:56AM -0400:
 On 09/08/2014 05:17 AM, Steven Hardy wrote:
 I think this may be a sensible move, but only if it's used primarily to
 land the less complex/risky patches more quickly.

 2 +2 has been part of OpenStack culture for a long time, and there is a
 good reason for it, it really does keep bugs out.

 It should also be clear that the subject of this email really should
 have been merging code faster, because nothing in here doubles the
 review bandwidth, it just provides us with less review coverage.
 
 For these reasons, I'm also wary of changing this in general.
 
 Sometimes I yell in IRC if I +2 something important, this shortens loop
 time as hopefully I already understand the patch and can answer
 questions.
 
 Single-approvals for small + simple changes could be worth trying.
 Perhaps also for large + simple changes like whitespace fixes.

Realistically core teams are already doing that. Being considered an
exception people typically provide a good reason in the review as to why
it's a fast approve, which is fine.

The large cross cutting whitespace fixes are actually completely
problematic in their own way, because they typically force a non trivial
rebase on 10s - 100s of patches in flight, so they generate *a ton* of
work for other people.

-Sean

-- 
Sean Dague
http://dague.net

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Kevin L. Mitchell
 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)

I'm going to make a tangential but somewhat related suggestion.  Instead
of reducing the number of +2s required, what would happen if we gave our
more trusted but non-core reviewers the ability to +2, but leave +A in
the hands of the existing cores?  That way, their reviews can be counted
by the core reviewers.  With this change in policy, you still need two
+2s, but you have more people that can +2, and you only need one of our
limited number of cores to review.
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-08 Thread Zane Bitter

On 07/09/14 23:43, Morgan Fainberg wrote:

## avoiding collaboration between bad actors

The two core requirement means that it takes three people (proposer +
2 core) to collaborate on landing something inappropriate (whether its
half baked, a misfeature, whatever). Thats only 50% harder than 2
people (proposer + 1 core) and its still not really a high bar to
meet. Further, we can revert things.

Solid assessment. I tend to agree with this point. If you are going to have bad 
actors try and get code in you will have bad actors trying to get code in. The 
real question is: how many (if any) extra reverts will be needed in the case of 
bad actors? My guess is 1 per bad actor (which that actor is likely no longer 
going to be core), if there are even any bad actors out there.


I think this misses the point, which isn't so much to prevent bad actors 
(and I don't think we have any of those). It's to protect good (and 
sometimes maybe slightly misguided) actors from any perception that they 
might be behaving as bad actors.


I think Rob missed another possible benefit off the list: it allows us 
to add core team members more aggressively than we might if adding 
someone meant allowing them to approve patches by themselves.


I'm not convinced that dropping the 2 x +2 is the right trade-off, 
though I would definitely support more official documentation of the 
wiggle room available for reviewer discretion, such as what Flavio 
suggested. In Heat we agreed on a policy of allowing immediate approval 
in the case where you're effectively reviewing a rebase of or a minor 
fix to a patchset that already had the assent in principle of two core 
reviewers. I rarely see anyone actually do it though, I think in part 
because the OpenStack-wide documentation makes it sound very naughty. I 
was interested to learn from this thread that many programs appear to 
have informally instituted something similar.


cheers,
Zane.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-07 Thread Angus Salkeld
On Mon, Sep 8, 2014 at 1:14 PM, Robert Collins robe...@robertcollins.net
wrote:

 I hope the subject got your attention :).

 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.

 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)


And you can always use +1, if you feel that another core should review.

-Angus


 Thats it really - below I've some arguments to support this suggestion.

 -Rob

 # Costs of the current system

 Perfectly good code that has been +2'd sits waiting for a second +2.
 This is a common complaint from folk suffering from review latency.

 Reviewers spend time reviewing code that has already been reviewed,
 rather than reviewing code that hasn't been reviewed.

 # Benefits of the current system

 I don't think we gain a lot from the second +2 today. There are lots
 of things we might get from it:

 - we keep -core in sync with each other
 - better mentoring of non-core
 - we avoid collaboration between bad actors
 - we ensure -core see a large chunk of the changes going through the system
 - we catch more issues on the code going through by having more eyeballs

 I don't think any of these are necessarily false, but equally I don't
 they are necessarily true.

 ## keeping core in sync

 For a team of (say) 8 cores, if 2 see each others comments on a
 review, a minimum of 7 reviews are needed for a reviewer R's thoughts
 on something to be disseminated across the team via osmosis. Since
 such thoughts probably don't turn up on every review, the reality is
 that it may take many more reviews than that: it is a thing, but its
 not very effective vs direct discussion.

 ## mentoring of non-core

 This really is the same as the keeping core in sync debate, except
 we're assuming that the person learning has nothing in common to start
 with.

 ## avoiding collaboration between bad actors

 The two core requirement means that it takes three people (proposer +
 2 core) to collaborate on landing something inappropriate (whether its
 half baked, a misfeature, whatever).  Thats only 50% harder than 2
 people (proposer + 1 core) and its still not really a high bar to
 meet. Further, we can revert things.

 ## Seeing a high % of changes

 Consider nova -
 http://russellbryant.net/openstack-stats/nova-reviewers-90.txt
 Core team size: 21 (avg 3.8 reviews/day) [79.8/day for the whole team]
 Changes merged in the last 90 days: 1139 (12.7/day)

 Each reviewer can only be seeing 30% (3.8/12.7) of the changes to nova
 on average (to keep up with 12/day landing). So they're seeing a lot,
 but there's more that they aren't seeing already. Dropping 30% to 15%
 might be significant. OTOH seeing 30% is probably not enough to keep
 up with everything on its own anyway - reviewers are going to be
 hitting new code regularly.

 ## Catching more issues through more eyeballs

 I'm absolutely sure we do catch more issues through more eyeballs -
 but what eyeballs look at any given review is pretty arbitrary. We
 have a 30% chance of any given core seeing a given review (with the
 minimum of 2 +2s). I don't see us making a substantial difference to
 the quality of the code that lands via the second +2 review. I observe
 that our big themes on quality are around systematic changes in design
 and architecture, not so much the detail of each change being made.


 -Rob


 --
 Robert Collins rbtcoll...@hp.com
 Distinguished Technologist
 HP Converged Cloud

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] doubling our core review bandwidth

2014-09-07 Thread Morgan Fainberg
Responses in-line.


-Original Message-
From: Robert Collins robe...@robertcollins.net
Reply: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.org
Date: September 7, 2014 at 20:16:32
To: OpenStack Development Mailing List openstack-dev@lists.openstack.org
Subject:  [openstack-dev] doubling our core review bandwidth

 I hope the subject got your attention :).
  
 This might be a side effect of my having too many cosmic rays, but its
 been percolating for a bit.
  
 tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
 use 'needs 1x+2'. We can ease up a large chunk of pressure on our
 review bottleneck, with the only significant negative being that core
 reviewers may see less of the code going into the system - but they
 can always read more to stay in shape if thats an issue :)
  
 Thats it really - below I've some arguments to support this suggestion.
  
 -Rob

I think that this is something that can be done on a project-by-project basis. 
However, I don’t disagree that
the mandate could be moved to “must have 1x+2” but leave it to the individual 
projects to specify how that is
implemented.

 # Costs of the current system
  
 Perfectly good code that has been +2'd sits waiting for a second +2.
 This is a common complaint from folk suffering from review latency.
  
 Reviewers spend time reviewing code that has already been reviewed,
 rather than reviewing code that hasn't been reviewed.

This is absolutely true. There are many times things linger with a single +2 
and then become painful due to rebase
needs. This issue can be extremely frustrating (especially to newer 
contributors).

 # Benefits of the current system
  
 I don't think we gain a lot from the second +2 today. There are lots
 of things we might get from it:
  
 - we keep -core in sync with each other
 - better mentoring of non-core
 - we avoid collaboration between bad actors
 - we ensure -core see a large chunk of the changes going through the system
 - we catch more issues on the code going through by having more eyeballs
  
 I don't think any of these are necessarily false, but equally I don't
 they are necessarily true.


 ## keeping core in sync
  
 For a team of (say) 8 cores, if 2 see each others comments on a
 review, a minimum of 7 reviews are needed for a reviewer R's thoughts
 on something to be disseminated across the team via osmosis. Since
 such thoughts probably don't turn up on every review, the reality is
 that it may take many more reviews than that: it is a thing, but its
 not very effective vs direct discussion.

I wouldn’t discount how much benefit is added by forcing the cores to see more 
of the code going into the repo. I personally feel like (as a core on a 
project) I would be lacking a lot of insight as to the code base without the 
extra reviews. It might take me longer to get up to speed when reviewing or 
implementing something new simply because I have a less likely chance to have 
seen the recently merged code.

Losing this isn’t the end of the world by any means.

 ## mentoring of non-core
  
 This really is the same as the keeping core in sync debate, except
 we're assuming that the person learning has nothing in common to start
 with.

This isn’t really a benefit to multiple core reviewers looking over a patch set 
from my experience. Most of the mentoring I see has been either in IRC or just 
because reviews (non-core even) occur. I agree with your assessment.

 ## avoiding collaboration between bad actors
  
 The two core requirement means that it takes three people (proposer +
 2 core) to collaborate on landing something inappropriate (whether its
 half baked, a misfeature, whatever). Thats only 50% harder than 2
 people (proposer + 1 core) and its still not really a high bar to
 meet. Further, we can revert things.

Solid assessment. I tend to agree with this point. If you are going to have bad 
actors try and get code in you will have bad actors trying to get code in. The 
real question is: how many (if any) extra reverts will be needed in the case of 
bad actors? My guess is 1 per bad actor (which that actor is likely no longer 
going to be core), if there are even any bad actors out there.

 ## Seeing a high % of changes
  
 Consider nova - 
 http://russellbryant.net/openstack-stats/nova-reviewers-90.txt  
 Core team size: 21 (avg 3.8 reviews/day) [79.8/day for the whole team]
 Changes merged in the last 90 days: 1139 (12.7/day)
  
 Each reviewer can only be seeing 30% (3.8/12.7) of the changes to nova
 on average (to keep up with 12/day landing). So they're seeing a lot,
 but there's more that they aren't seeing already. Dropping 30% to 15%
 might be significant. OTOH seeing 30% is probably not enough to keep
 up with everything on its own anyway - reviewers are going to be
 hitting new code regularly.

 ## Catching more issues through more eyeballs
  
 I'm absolutely sure we do catch more issues through more eyeballs -
 but what