Where have all the reviewers gone?
So reviewers are a fundamental part of the process. They are also, it seems, somewhat scarce. Consider a couple of examples:
- In the kernel space, the reiser4 filesystem has been held up for some
time. There are many reasons for that delay, but one of those has been the
lack of a thorough review by somebody who understands the Linux
virtual filesystem layer well. Greg Kroah-Hartman, in his OLS
keynote, said, more generally: "
The big problem ... is we really only have a very small group of people reviewing code in the kernel community.
" - The PostgreSQL developers have been engaged in a lengthy discussion on
the upcoming 8.2 release, why it is taking as long as it is, and why
this release appears (to them) to have little in the way of exciting
new features. The conversation has touched on various aspects of that
project's development process; there are many things for those
developers to think about. One of them, though, as expressed by one of the participants, is:
"
...the real problem seems to be we do not have enough patch reviewers.
"
If we truly believe that code review is a crucial part of the free software process (and, for the most part, it is likely that we do believe this), then the idea that projects are being slowed by the lack of reviewers is a bit worrying. At best, a reviewer shortage will be a bottleneck in the process; a worse possibility is that some projects will simply decide to do without.
Reviewers serve a number of purposes. They can often immediately spot that bug that the developer has stared at for hours without finding. If the code is hard to understand, the reviewers will be the first to notice. If the associated documentation is incorrect or (as is more often the case) absent, the reviewers will notice that as well. When code appears to have been written using some sort of specialized, non-public knowledge, reviewers can inquire as to its provenance. Coding style issues, API misuse, inefficient algorithms, use of outdated interfaces, and more can be caught in the review process before the code hits the project's mainline. Reviewers really do increase a project's code quality and long-term maintainability.
The problem is that code review can be a difficult, tiring, and thankless job. Human nature being what it is, people will often show less than the appropriate amount of gratitude when a reviewer points out their mistakes in public. This is especially true if the code has problems which will require significant amounts of work to fix. The reviewer did not create these problems, he or she is simply the messenger with the bad news. So reviewers tend to get grumpy, especially when they see the same mistakes being made over and over again.
Developers get credit for their work, in various forms. It is a rare project release, however, which publicly acknowledges those who reviewed the code. Given that writing code is not only a more visible activity, but it also tends to be more fun than reviewing code written by others, it is not surprising that many developers choose to concentrate on their own work.
Finally, reviewing code can be intimidating - especially if the patch of interest has a Big Name behind it. Many potential reviewers may feel that they simply do not have the standing to poke at other peoples' work. The fact is, however, that even people with a relatively small amount of experience can provide useful reviews, and learn from the process. From Greg's OLS keynote:
If we want to create the best free systems we can, we must ensure that the
review portion of the process does not get slighted. To that end, people
who have the requisite skills would do well to dedicate a bit of their time
to reviewing code in a project that interests them. Buy a reviewer a beer,
and forgive them if they tell you, in front of hundreds or thousands of
developers, that your work is best suited for a place in the project's "bad
examples" repository. Listen to what the reviewers say, respond to it, and
thank them. The result will be better software for all of us.
| Index entries for this article | |
|---|---|
| Kernel | Development model/Code review |
(Log in to post comments)
Goofy analogy
Posted Sep 14, 2006 2:38 UTC (Thu) by autarch (subscriber, #22025) [Link]
I just have to comment on that analogy to writing music. Most people who learn an instrument, in particular a classical instrument, will never write any music. In the classical world, composition is generally a separate discipline.
Goofy analogy
Posted Sep 14, 2006 10:15 UTC (Thu) by jonth (guest, #4008) [Link]
Think about it the other way around: How many composers can you think of who are unable to play any instrument?
Goofy analogy
Posted Sep 14, 2006 10:17 UTC (Thu) by tykepenguin (subscriber, #4346) [Link]
I can (but won't) name two.
Mind you, they write rubbish ;-)
Goofy analogy
Posted Sep 14, 2006 13:31 UTC (Thu) by NRArnot (subscriber, #3033) [Link]
In another way it's a good analogy. I can't compose music like Chopin, but I can notice that a note in the sheet music is a wrong one. When I did, comparison with another edition revealed that it was a copyist's error, but composers' manuscripts themselves often show plenty of corrections. Being a great composer doesn't always mean an unerring ability to write down every note as intended first time.
And programming is exactly like that. A mere student can spot a bug that an experienced programmer will miss, because the experienced programmer knows what the code ought to be doing without studying the fine detail, and the student does not.
Where have all the reviewers gone?
Posted Sep 14, 2006 4:04 UTC (Thu) by pimlott (guest, #1535) [Link]
While the points about both the value and difficulty of code review are well placed, the title of this article is misleading: The reviewers have not gone anywhere, because they were never here.Our code, we claim, is better because it has been reviewed and improved by a variety of people beyond the original author(s).I'll let Al Viro answer that, as he did to Eric Raymond five years ago:
* you've made a completely unwarranted assumption - that widely-used and available code actually gets reviewed by many people. It's demonstrably false.If projects are being held up by scarce reviewing resources, that's a wonderful sign that leaders are insisting upon review, not an indicator of any decline in those resources.
Where have all the reviewers gone?
Posted Sep 15, 2006 9:04 UTC (Fri) by jbailey (guest, #16890) [Link]
Aren't most of ESR's statements demonstrably false? He likes to paint utopian hacker worlds that I have yet to see or meet.
The fact that people still quote him in general always mystifies me.
Where have all the reviewers gone?
Posted Sep 15, 2006 9:08 UTC (Fri) by jbailey (guest, #16890) [Link]
While not talking about the kernel specifically, a problem with being a reviewer is that all of us start as bad reviewers and then have to progress to being good reviewers. When reviewing, we're not trying to match our own style and habits, but those of other people.
It's hard to provide a convincing argument to hackers that they should mentor reviewers. It's a bad hill of dominoes: The maintainer probably isn't good at mentoring, they're responsible for a reviewer who's not good at reviewing, who's reviewing crap code coming in.
I don't know of a good way to fix that, really. =/
Where have all the reviewers gone?
Posted Sep 14, 2006 7:03 UTC (Thu) by dark (guest, #8483) [Link]
I think you should distinguish between review before a patch is accepted, and review after code has been published and is in the hands of users. Only the latter is what I would call "peer review" -- since it's independent -- and it's what the often-proclaimed advantage refers to: our code is public. And that's the published code, not proposed code on a mailing list.
Otherwise, proprietary software could have the same kind of "peer review", and many of them do have proposed code reviewed by fellow developers before it goes in. (Well, ok. Some of them. :-)
Where have all the reviewers gone?
Posted Sep 14, 2006 7:54 UTC (Thu) by tjasper (subscriber, #4310) [Link]
I think this is a valid point. Whilst the quality of code review before the code gets accepted is raised by good reviews, if the code gets out and is still buggy, then users with enough of an itch will patch it so it works for them.
So maybe the balance has to lie somewhere between perfect code which has been reviewed to death so that there are no bugs left!!! and obviously buggy code which would bite users too much. I think the community needs to recognise that stifling new code because no-one can review it deprives the community of it's best weapon - all the users who find bugs and help fix them. Sure, overly buggy code scares users away, but there is a balance - which is probably different for every project......
Hmmmm.....
Where have all the reviewers gone?
Posted Sep 14, 2006 8:04 UTC (Thu) by tjasper (subscriber, #4310) [Link]
Consider further that Linus himself has stated that some of his very early code was ugly IIRC! So it does it have to be perfect every time? It was Linus' release early, release often approach which excited people as they saw new stuff appearing quickly.
The new kernel development model does the same, and it may pay for bigger projects to look for ways to get new code out there being used by people who find bugs through use rather than review before the code is released.
Also, the Coverity checker seems to be doing a good job of improving code quality without the backlash from the authors (hard to insult a program!! :o) ). Long may that continue.
Where have all the reviewers gone?
Posted Sep 14, 2006 22:39 UTC (Thu) by dlang (guest, #313) [Link]
this is a very important point
reviewing patches to see if they are acceptable requires not just someone who can understand code, but also somone who's judgement is trusted by the project. Earning that trust is hard, and people who do are frequently authors of code rather then reviewers of it.
on the other hand it takes no credentials to review code once it's been published. anyone can do it, and if they spot a problem they can report it (with or without a patch, it's perfectly fine to say 'hey, this looks wrong' even without a patch to fix it, these are bug reports, not feature requests)
this doesn't mean that the number of reviewers of both types can't be increased, we need more of both types, but the problem isn't the pending disaster this article implies.
Code review and RSI
Posted Sep 14, 2006 7:23 UTC (Thu) by riddochc (guest, #43) [Link]
This has been a topic on my mind recently.I have RSI problems, and have recently taken to doing more paper programming than keyboarding. I'm also "between jobs," and in the course of asking friends how I could make money without typing so much, the suggestion of code review was made.
And yet I've seen remarkably fewer positions open for code reviewers than for programmers. Take my wild speculation for what it is, but this suggests to me that the amount of review done on code may not be substantially better in proprietary settings than in the open source community.
I'll try my hand at reviewing the code for something (I haven't decided what, yet) but at the risk of being off-topic, I ask the LWN crowd, being collectively much older and wiser than I:
What kinds of jobs could the typical longtime LWN-reader hold, having RSI, that wouldn't require switching fields and throwing away years of computer experience?
Code review and RSI
Posted Sep 14, 2006 15:18 UTC (Thu) by tjc (guest, #137) [Link]
And yet I've seen remarkably fewer positions open for code reviewers than for programmers.I worked at a lot of places, some of them quite large, but I've never been anywhere that hired people for the sole purpose of code review. It's almost always done by another progammer, usually with indifference and in great haste, if it's done at all. The only things worse are 4-hour requirements meetings, and writing documentation.
Code review and RSI
Posted Sep 15, 2006 19:40 UTC (Fri) by rountree (guest, #29277) [Link]
Academia can be welcoming. I'm aware of one situation where a professor accepted a position subject to being provided with a part-time typist by the university. As the work-study pool is pretty large, it works out to be a cheap benefit.
It might be possible to get that setup as a grad student as well.
Where have all the reviewers gone?
Posted Sep 14, 2006 7:51 UTC (Thu) by eskild (guest, #1556) [Link]
I would like to add that in the commercial world, things are often no better; in fact, they're often worse. I've held jobs in several companies now, from small to medium to huge -- and while there's often been a desire for review, I've never seen it enacted except for one case where problems were so bad that it was unavoidable.
In other words: Good intentions, but it just doesn't happen unless brownish stuff hits the fan.
Far more common is the kind of "review" that takes place when someone else has to look at the code during normal maintenance. But that typically only touches the narrow area where modifications are originally required; and it is late in the lifecycle of the code, so it's very hard to really "do something about it" if there are problems.
One thing underlying it is, IMHO, that the quality of software is usually determined from external observation: "Does it work?" -- where the question really ought to be: "Does it work, and is it built well?"
Where have all the reviewers gone?
Posted Sep 14, 2006 8:41 UTC (Thu) by NAR (subscriber, #1313) [Link]
in the commercial world, things are often no betterIn my experience there is peer review in the commercial world, altough not as much as it should be. In the previous two projects I've worked the version control system was set up so every modification to the code was sent via e-mail to the interested parties (e.g. senior developers), who had the oppurtunity to check it. Also the project management pressured the developers to hold formal code reviews for new functionality.
Proprietary side of the coin
Posted Sep 14, 2006 12:43 UTC (Thu) by rvfh (guest, #31018) [Link]
I remember my dear friend Zoon did that in our company, so he and I would see what our less experienced colleagues were checking in.
The problem was then to make them understand why a particular change was bad, and should be corrected, and with time pressure on the project, even the managers were just biting the bullet in the end, and the crap stayed there accumulating.
Mind you, we both left the company quickly after that!
Where have all the reviewers gone?
Posted Sep 14, 2006 10:49 UTC (Thu) by nlucas (subscriber, #33793) [Link]
I'm not really sure if one can say peer review happens on the majority of the open source projects. Yes, all the major projects do it, but what about the smaller ones?
The majority of the Open Source projects are small projects with 0,1, maybe 2 active developpers, and I would dare to say most are just begining to learn how to program (that's why they started the project, so they could get comments that would allow them to learn more).
It's true most of this projects will not be found on the debian repository, but even if they are "weak" that doesn't make them less open source than the major players.
If they are good (and with some luck), they can attract enough attention from the big guys and thus get a good code review, but I would say most of them become just one more dead project on sourceforge (which in itself is not bad, because others can use it's "hashes" as a starting point).
Where have all the reviewers gone?
Posted Sep 16, 2006 1:59 UTC (Sat) by liamh (guest, #4872) [Link]
A more apt analogy may be a journal's use of reviewers (referees) for professional papers. These are unpaid jobs, and typically an editor has to beg and cajole qualified people to spend time to do a review. And often, the paper sits on the referee's desk for weeks/months/years before being reviewed, or before the referee/editor decides he hasn't the time to do it and it needs to be passed on to someone else. In principle, the referee earns good feelings from the editor and thus gains stature in the community, but that's pretty intangible.
On the other hand, the journals have solved the problem of recriminations on referees for negative reviews. For the most part, referees are anonymous.
Time for the GNU Hurd?
Posted Sep 21, 2006 10:37 UTC (Thu) by leandro (guest, #1460) [Link]
While the story doesn’t focus on kernels (Linux) only, but also on DBMSs and such, perhaps it shows it is time for rethinking.
In the OS space, this rethinking has long been done, but until now been too much radical to gain a practical hold. The Hurd is perennially almost done, but it was conceived to solve precisely this problem: internalize some complexity in the architecture of the system, to simplify its interfaces, so that changes are better contained thus both simplifying development (and thus code review) and limiting side effects of new development (thus both simplifying code review, and making it less critical).
In the DBMS space, SQL is too complex, full of arbitrary limitations, inefficiencies, redundant-but-slightly-different ways of doing stuff and so on. It has been some years now the Third Manifesto proposed a radically simpler, more powerful class of D data languages, of which there are several prototypes and even a full-scale if proprietary and slightly deviant system.
Finally, the language problem. Until know being based on a functional language is considered an oddity, limiting the amount of talent available for Lisp (for instance) based projects. But given the simplicity of Lisp, and specially Scheme, one hopes Guile will become more widespread as an extension library, and Lisp as a systems programming language. Now we already have GNU Emacs, GNUCash, the Gimp, Sawfish; ideally all these would standardise on the Guile, and on the other hand GCL would become more widespread and fix its deficiencies.
By the way, the Hurd is but a stepping stone for a Lisp future.
And, it's still a problem a year later...
Posted May 24, 2007 22:42 UTC (Thu) by JesseW (subscriber, #41816) [Link]
LWN has a second article on this topic, Where have the reviewers gone? (May 22, 2007)