<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">
<div apple-content-edited="true"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">
<br>
</div>
</span></span></div>
<div>
<div>On Oct 16, 2013, at 11:19 , Robert Collins <<a href="mailto:robertc@robertcollins.net">robertc@robertcollins.net</a>></div>
<div> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">On 16 October 2013 20:14, Alessandro Pilotti<br>
<<a href="mailto:apilotti@cloudbasesolutions.com">apilotti@cloudbasesolutions.com</a>> wrote:><br>
<blockquote type="cite"><br>
</blockquote>
<br>
<blockquote type="cite">Drivers are IMO not part of the core of Nova, but completely separated and decoupled entities, which IMO should be treated that way. As a consequence, we frankly don't stricly feel as part of Nova, although some of us have a pretty strong
understanding of how all the Nova pieces work.<br>
</blockquote>
<br>
I don't have a particular view on whether they *should be* separate<br>
decoupled entities, but today I repeatedly hear concerns about the<br>
impact of treating 'internal APIs' as stable things. That's relevant<br>
because *if* nova drivers are to be separate decoupled entities, the<br>
APIs they use - and expose - have to be treated as stable things with<br>
graceful evolution, backwards compatibility etc. Doing anything else<br>
will lead to deployment issues and race conditions in the gate.<br>
<br>
<blockquote type="cite">Obliging driver (or other decoupeld sub-components) developers to learn the entire Nova project before being able to contribute would just kill their effort before the start, resulting in a poorer ecosystem.<br>
</blockquote>
<br>
There are lots of different aspects of contribution: reviews (as<br>
anybody), reviews (as core), code, bug assessment, design input,<br>
translations, documentation etc. Nobody has said that you have to<br>
learn everything before you contribute.<br>
<br>
The /only item/ in that list that requires wide knowledge of the code<br>
base is reviews (as core).<br>
<br>
The difference between reviews (as anybody) and reviews (as core) is<br>
that core is responsible for identifying things like duplicated<br>
functionality, design and architectural issues - and for only<br>
approving a patch when they are comfortable it doesn't introduce such<br>
issues.<br>
<br>
Core review is a bottleneck. When optimising systems with a bottleneck<br>
there are only three things you can do to make the system work better:<br>
- avoid the bottleneck<br>
- increase capacity of the bottleneck<br>
- make the bottleneck more efficient at the work it's given<br>
<br>
Anything else will just end up backing up work behind the bottleneck<br>
and make /no/ difference at all.<br>
<br>
Your proposal (partition the code & reviewers by core/drivers) is an<br>
'avoid the bottleneck' approach. It will remove some fraction of<br>
reviews from the bottleneck - those that are per driver - at the cost<br>
of losing /all/ of the feedback, architecture and design input that<br>
the drivers currently benefit from, *and* removing from the nova core<br>
any insight into the issues drivers are experiencing (because they<br>
won't be reviewing driver code). Unless you have 'in-tree' and<br>
'out-of-tree' drivers, and then one has to ask 'why are some drivers<br>
out of tree'? The only answer for that so far is 'review latency is<br>
hurting drivers'... but review latency is hurting *everyone*.<br>
<br>
To increase bottleneck capacity we could ask -core reviewers to spend<br>
more time reviewing. However that doesn't work because we're human -<br>
we need time to do other things than think hard about other peoples<br>
code. There is an upper bound on effective time spent reviewing by<br>
reviewers. Or we can increase capacity of the bottleneck by training<br>
up more -core reviewers. Thats pretty obvious :). However training up<br>
more reviewers requires more reviewers - so the cost here is that<br>
someone needs to volunteer that time.<br>
<br>
The bottleneck - core reviewers - can get through more reviews when<br>
the reviews are easy to do. From prior conversations here is my list:<br>
- keep the change small. Lots of LOC == a hard review, which consumes<br>
-core review time<br>
- get the review reviewed by non-core as soon as you put it up - this<br>
will catch many issues -core would and reduce re-work<br>
- follow the style guides<br>
- make your commit message really clear - there's a style guide for these too!<br>
<br>
It seems to me that one can order the choices by their costs:<br>
- provide patches that are easier to review [basically no cost: the<br>
rules are already in place]<br>
- train more -core reviewers [needs volunteer time]<br>
- split the drivers out [lose coherency on tightly coupled things,<br>
have to stabilise more interfaces, lose experienced input into driver<br>
code]<br>
<br>
And by their effectiveness [this is more subjective:)]<br>
- train more -core reviewers [essentially linear, very easy to predict]<br>
- provide patches that are easier to review [many patches are good<br>
already, has a low upper bound on effectiveness]<br>
- split the drivers out [won't help *at all* with changes required in<br>
core to support a driver feature]<br>
<br>
Finally, there is a key thing to note: As contributors to the project<br>
scales, patch volume scales. As patch volume scales the pressure on<br>
the bottleneck increases: we *have* to scale the -core review team [or<br>
partition the code bases into two that will **both have a solid<br>
reviewer community**].<br>
<br>
Remember that every patch added requires *at minimum* 2 core reviews<br>
[and I suspect in reality 3 core reviews - one to catch issues, then a<br>
re-review, then a second and land]. This gives a pretty simple<br>
equation for code contributors: if you upload a patch, to keep things<br>
equitable, you need to do at least 3 reviews of other peoples patches.<br>
While you're not -core these reviews will prepare other peoples<br>
patches for -core attention (saving -core bandwidth). If you are<br>
-core, then you'll be helping get other patches out of the way so that<br>
your own can be seen by another -core.<br>
<br>
That said, in the last 6 months - and please don't take this as<br>
calling you out, I'd be happy to do this for any contributor -<br>
| alexpilotti | 22 0 8 14 0 63.6% | 2<br>
( 14.3%) |<br>
and<br>
git log --author Pilotti --since=2013-04-04 | grep commit | wc -l<br>
10<br>
<br>
</blockquote>
<div><br>
</div>
<div>Robert, as I wrote in a previous email, I'm moving gradually away from submitting patches and concentrate on reviewing code mainly on my team's work across all projects on which we are involved (Nova, Neutron, Cinder, Ceilometer, plus the external ones
like Cloudbase-Init, Crowbar, etc).</div>
<div><br>
</div>
<div>So, hopefully my work will consist almost entirely in reviews pretty soon, starting probably already with Icehouse.</div>
<div>You still won't see large volumes of reviews on a single project, because they will be spread across multiple ones.</div>
<div><br>
</div>
<div><br>
</div>
<blockquote type="cite">So - you're doing 2 reviews for every patch of your own that lands,<br>
which is fine, if your patches are going in with no re-reviews by<br>
-core. But whether that happens is somewhat inconsistent -<br>
<a href="https://review.openstack.org/#/c/38160/">https://review.openstack.org/#/c/38160/</a> took 11 -core reviews.<br>
<a href="https://review.openstack.org/#/c/38160/">https://review.openstack.org/#/c/38160/</a> took 9 -core reviews.<br>
<a href="https://review.openstack.org/#/c/43197/">https://review.openstack.org/#/c/43197/</a> took 4.
</blockquote>
<div><br>
</div>
<div>Excellent examples. If you look at the reviews, you'll see countless rebases, requests of splitting a big patch in smaller patches with subsequent rebase hell and cascading re-reviews. Why so many? Because (for example) the first one got there on July
22nd and merged on September 3rd!</div>
<div><br>
</div>
<div>Which means 43 days!!! forty-three-days!!!</div>
<div><br>
</div>
<div><br>
</div>
<div>Basically in most of the patches that we committed the code landed w/o any significat changes, excluding notes on unit tests and minor formal stuff.</div>
<div><br>
</div>
<div>Why reviews were limited to unit tests and formalities? Of course because that code, for Nova reviewers, is simply "black magic" as nobody in Nova has the slightest clue about Hyper-V internals.</div>
<div><br>
</div>
<div>Not only this hell disrupted completely our Havana development cycle, but it even wasted lots of core reviewers time in reviewing stuff for nothing due to the cascading rebasings since most blueprints were dependent on others.</div>
<div><br>
</div>
<div>So sorry, but I won't definitely count those 11, 9 or whatever reviews as significant for anything more than an example of awful project management :-)</div>
<div><br>
</div>
<br>
<blockquote type="cite">So I think to meet the<br>
quid-pro-quo you really need to be doing 7-8 reviews for every patch<br>
you upload. *ignore* discussions about what it takes to be -core: lets<br>
start by meeting the social contract we have in OpenStack, that review<br>
is a shared workload shared by all the contributors. We all need to do<br>
this, otherwise we end up with asymmetric review load and thats what<br>
leads to the current situation with a huge fraction of reviews being<br>
done by a tiny fraction of the reviewers.<br>
<br>
-Rob<br>
<br>
<br>
-- <br>
Robert Collins <<a href="mailto:rbtcollins@hp.com">rbtcollins@hp.com</a>><br>
Distinguished Technologist<br>
HP Converged Cloud<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev<br>
</blockquote>
</div>
<br>
</body>
</html>