Open Stack

On Oct 16, 2013, at 11:19 , Robert Collins <robertc at robertcollins.net<mailto:robertc at robertcollins.net>>
 wrote:

On 16 October 2013 20:14, Alessandro Pilotti
<apilotti at cloudbasesolutions.com<mailto:apilotti at cloudbasesolutions.com>> wrote:>


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.

I don't have a particular view on whether they *should be* separate
decoupled entities, but today I repeatedly hear concerns about the
impact of treating 'internal APIs' as stable things. That's relevant
because *if* nova drivers are to be separate decoupled entities, the
APIs they use - and expose - have to be treated as stable things with
graceful evolution, backwards compatibility etc. Doing anything else
will lead to deployment issues and race conditions in the gate.

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.

There are lots of different aspects of contribution: reviews (as
anybody), reviews (as core), code, bug assessment, design input,
translations, documentation etc. Nobody has said that you have to
learn everything before you contribute.

The /only item/ in that list that requires wide knowledge of the code
base is reviews (as core).

The difference between reviews (as anybody) and reviews (as core) is
that core is responsible for identifying things like duplicated
functionality, design and architectural issues - and for only
approving a patch when they are comfortable it doesn't introduce such
issues.

Core review is a bottleneck. When optimising systems with a bottleneck
there are only three things you can do to make the system work better:
- avoid the bottleneck
- increase capacity of the bottleneck
- make the bottleneck more efficient at the work it's given

Anything else will just end up backing up work behind the bottleneck
and make /no/ difference at all.

Your proposal (partition the code & reviewers by core/drivers) is an
'avoid the bottleneck' approach. It will remove some fraction of
reviews from the bottleneck - those that are per driver - at the cost
of losing /all/ of the feedback, architecture and design input that
the drivers currently benefit from, *and* removing from the nova core
any insight into the issues drivers are experiencing (because they
won't be reviewing driver code). Unless you have 'in-tree' and
'out-of-tree' drivers, and then one has to ask 'why are some drivers
out of tree'? The only answer for that so far is 'review latency is
hurting drivers'... but review latency is hurting *everyone*.

To increase bottleneck capacity we could ask -core reviewers to spend
more time reviewing. However that doesn't work because we're human -
we need time to do other things than think hard about other peoples
code. There is an upper bound on effective time spent reviewing by
reviewers. Or we can increase capacity of the bottleneck by training
up more -core reviewers. Thats pretty obvious :). However training up
more reviewers requires more reviewers - so the cost here is that
someone needs to volunteer that time.

The bottleneck - core reviewers - can get through more reviews when
the reviews are easy to do. From prior conversations here is my list:
- keep the change small. Lots of LOC == a hard review, which consumes
-core review time
- get the review reviewed by non-core as soon as you put it up - this
will catch many issues -core would and reduce re-work
- follow the style guides
- make your commit message really clear - there's a style guide for these too!

It seems to me that one can order the choices by their costs:
- provide patches that are easier to review [basically no cost: the
rules are already in place]
- train more -core reviewers [needs volunteer time]
- split the drivers out [lose coherency on tightly coupled things,
have to stabilise more interfaces, lose experienced input into driver
code]

And by their effectiveness [this is more subjective:)]
- train more -core reviewers [essentially linear, very easy to predict]
- provide patches that are easier to review [many patches are good
already, has a low upper bound on effectiveness]
- split the drivers out [won't help *at all* with changes required in
core to support a driver feature]

Finally, there is a key thing to note: As contributors to the project
scales, patch volume scales. As patch volume scales the pressure on
the bottleneck increases: we *have* to scale the -core review team [or
partition the code bases into two that will **both have a solid
reviewer community**].

Remember that every patch added requires *at minimum* 2 core reviews
[and I suspect in reality 3 core reviews - one to catch issues, then a
re-review, then a second and land]. This gives a pretty simple
equation for code contributors: if you upload a patch, to keep things
equitable, you need to do at least 3 reviews of other peoples patches.
While you're not -core these reviews will prepare other peoples
patches for -core attention (saving -core bandwidth). If you are
-core, then you'll be helping get other patches out of the way so that
your own can be seen by another -core.

That said, in the last 6 months - and please don't take this as
calling you out, I'd be happy to do this for any contributor -
|       alexpilotti        |      22    0   8  14   0    63.6% |    2
( 14.3%)  |
and
git log --author Pilotti  --since=2013-04-04 | grep commit | wc -l
10


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).

So, hopefully my work will consist almost entirely in reviews pretty soon, starting probably already with Icehouse.
You still won't see large volumes of reviews on a single project, because they will be spread across multiple ones.


So - you're doing 2 reviews for every patch of your own that lands,
which is fine, if your patches are going in with no re-reviews by
-core. But whether that happens is somewhat inconsistent -
https://review.openstack.org/#/c/38160/ took 11 -core reviews.
https://review.openstack.org/#/c/38160/ took 9 -core reviews.
https://review.openstack.org/#/c/43197/ took 4.

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!

Which means 43 days!!!  forty-three-days!!!


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.

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.

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.

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 :-)


So I think to meet the
quid-pro-quo you really need to be doing 7-8 reviews for every patch
you upload. *ignore* discussions about what it takes to be -core: lets
start by meeting the social contract we have in OpenStack, that review
is a shared workload shared by all the contributors. We all need to do
this, otherwise we end up with asymmetric review load and thats what
leads to the current situation with a huge fraction of reviews being
done by a tiny fraction of the reviewers.

-Rob


--
Robert Collins <rbtcollins at hp.com<mailto:rbtcollins at hp.com>>
Distinguished Technologist
HP Converged Cloud

_______________________________________________
OpenStack-dev mailing list
OpenStack-dev at lists.openstack.org<mailto:OpenStack-dev at lists.openstack.org>
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20131016/f033c4f3/attachment.html>