[openstack-dev] [nova] An analysis of code review in Nova

Matthew Booth mbooth at redhat.com
Tue Feb 25 12:36:52 UTC 2014


I'm new to Nova. After some frustration with the review process,
specifically in the VMware driver, I decided to try to visualise how the
review process is working across Nova. To that end, I've created 2
graphs, both attached to this mail.

Both graphs show a nova directory tree pruned at the point that a
directory contains less than 2% of total LOCs. Additionally, /tests and
/locale are pruned as they make the resulting graph much busier without
adding a great deal of useful information. The data for both graphs was
generated from the most recent 1000 changes in gerrit on Monday 24th Feb
2014. This includes all pending changes, just over 500, and just under
500 recently merged changes.

pending.svg shows the percentage of LOCs which have an outstanding
change against them. This is one measure of how hard it is to write new
code in Nova.

merged.svg shows the average length of time between the
ultimately-accepted version of a change being pushed and being approved.

Note that there are inaccuracies in these graphs, but they should be
mostly good. Details of generation here:
https://github.com/mdbooth/heatmap. This code is obviously
single-purpose, but is free for re-use if anyone feels so inclined.

The first graph above (pending.svg) is the one I was most interested in,
and shows exactly what I expected it to. Note the size of 'vmwareapi'.
If you check out Nova master, 24% of the vmwareapi driver has an
outstanding change against it. It is practically impossible to write new
code in vmwareapi without stomping on an oustanding patch. Compare that
to the libvirt driver at a much healthier 3%.

The second graph (merged.svg) is an attempt to look at why that is.
Again comparing the VMware driver with the libvirt we can see that at 12
days, it takes much longer for a change to be approved in the VMware
driver than in the libvirt driver. I suspect that this isn't the whole
story, which is likely a combination of a much longer review time with
very active development.

What's the impact of this? As I said above, it obviously makes it very
hard to come in as a new developer of the VMware driver when almost a
quarter of it has been rewritten, but you can't see it. I am very new to
this and others should validate my conclusions, but I also believe this
is having a detrimental impact to code quality. Remember that the above
12 day approval is only the time for the final version to be approved.
If a change goes through multiple versions, each of those also has an
increased review period, meaning that the time from first submission to
final inclusion is typically very, very protracted. The VMware driver
has its fair share of high priority issues and functionality gaps, and
the developers are motived to get it in the best possible shape as
quickly as possible. However, it is my impression that when problems
stem from structural issues, the developers choose to add metaphorical
gaffer tape rather than fix them, because fixing both creates a
dependency chain which pushes the user-visible fix months into the
future. In this respect the review process is dysfunctional, and is
actively detrimental to code quality.

Unfortunately I'm not yet sufficiently familiar with the project to
offer a solution. A core reviewer who regularly looks at it is an
obvious fix. A less obvious fix might involve a process which allows
developers to work on a fork which is periodically merged, rather like
the kernel.

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pending.svg
Type: image/svg+xml
Size: 12258 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140225/d0de8dd7/attachment.svg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: merged.svg
Type: image/svg+xml
Size: 12257 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140225/d0de8dd7/attachment-0001.svg>


More information about the OpenStack-dev mailing list