On Oct 16, 2013, at 11:19 , Robert Collins <robe...@robertcollins.net<mailto:robe...@robertcollins.net>> wrote:
On 16 October 2013 20:14, Alessandro Pilotti <apilo...@cloudbasesolutions.com<mailto:apilo...@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 <rbtcoll...@hp.com<mailto:rbtcoll...@hp.com>> Distinguished Technologist HP Converged Cloud _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org<mailto: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
