LWN Weekly Edition
Front pageSecurity
Kernel development
Distributions
Development
Announcements
->One big page
This page
Previous weekFollowing week
| |||||||||||||
DevelopmentName-and-shame to inspire patch reviewsNot many developers actually prefer reviewing patches; it is time-consuming and is often not as much fun as writing code. Nevertheless, it is an essential part of software development, so open source projects must find some way to motivate volunteers to undertake the task. PostgreSQL recently tried out a rather unusual approach: sending out a public list of all of those patch submitters who had not taken on a patch to review—which project policy mandates. The move ruffled a few feathers, but on the whole, more people seemed supportive of the idea than seemed offended by it. At the heart of the issue is the project's practice of holding CommitFests—periodic pauses in PostgreSQL development during which the focus changes from new development work to patch review and merging. There have been many such CommitFests in recent years; four per calendar year. The current one started on June 16, targeting the upcoming 9.4 release. Josh Berkus, the current CommitFest Manager (CFM), emailed the pgsql-hackers list to announce it, and included a reminder about the project's policy that everyone who submits a patch is expected to pitch in and review at least one patch by someone else. Berkus followed up on the subject on June 24, sending a list of the patch submitters who had not yet claimed a patch to review. The list was sizable, he said—at least as a proportion of the overall participants:
The two lists below, idle submitters and committers, constitutes 26
people. This *outnumbers* the list of contributors who are busy
reviewing patches -- some of them four or more patches. If each of
these people took just *one* patch to review, it would almost entirely
clear the list of patches which do not have a reviewer. If these folks
continue not to do reviews, this commitfest will drag on for at least 2
weeks past its closure date.
In addition, he noted, the patch-review policy (adopted for the PostgreSQL 9.2 development cycle) indicates that not helping with patch review would "affect the attention your patches received in the future." He further encouraged developers who submit patches for their employer to let their supervisors know that participating in the review process is considered part of contributing. Naming names?The tone of Berkus's email was certainly conciliatory, but there were nevertheless a few people on the mailing list who seemed taken aback by the act of publicly listing the names of the non-reviewers. Dimitri Fontaine said "I very much don't like that idea of publishing a list of names," and that he would prefer sending out individual notices. Andreas Karlsson concurred, arguing that private reminders are more effective. Berkus replied that private emails had been sent a week earlier, but only a few of the recipients had picked up a patch in the interim. And, as he noted later, although the "submit one, review one" policy had been in place for previous CommitFests, it had simply never been enforced in the past. Several of these early critics referred to the process as "naming and shaming," but Joshua Drake disagreed with that characterization:
I believe you are taking the wrong perspective on this. This is
not a shame wall. It is a transparent reminder of the policy and those
who have not assisted in reviewing a patch but have submitted a patch
themselves.
Drake quipped that participants should "leave the ego at the door," but Fontaine replied that ego was not the issue at all. Emailing the list of non-reviewers to everyone, he said, would discourage contributors from participating. Instead, he said, the project ought to do something positive to reward the people who do review patches. Maciej Gajewski did not object to publicizing the list of names, but argued that the policy itself needed to be more prominently communicated, so that "newbies like myself (who wouldn't even dare reviewing patches submitted be seasoned hackers) are not surprised by seeing own name on a shame wall." Let's reviewWhether or not sending the names to the mailing list constitutes "shaming" is, of course, a matter of opinion. Mark Kirkwood pointed out that the subject line of Berkus's email ("[9.4 CF 1] The Commitfest Slacker List") may have triggered that interpretation: "Lol - Josh's choice of title has made it a small shame wall (maybe only knee high)." Nevertheless, the content of Berkus's email simply reminded recipients of the review policy in pretty straightforward language. One might argue that working in the open dictates public scrutiny of all sorts of potentially embarrassing incidents; for instance, is it "shaming" to have a patch rejected? Furthermore, the only reason that it is important to remind list participants of the CommitFest review policy (and perhaps the key reason to have the policy in the first place) is that patch reviewers are in short supply relative to submitters, and the project is trying to rectify the imbalance. As Kirkwood's email pointed out, reviewing patches is a fundamentally more difficult task than submitting them:
I've submitted a few patches in a few
different areas over the years - however if I grab a patch on the queue
that is not in exactly one of the areas I know about, I'll struggle to
do a good quality review.
Now some might say "any review is better than no review"... I don't think so - one of my patches a while was reviewed by someone who didn't really know the context that well and made the whole process grind to a standstill until a more experienced reviewer took over. But, Tom Lane countered, a major reason for the existing CommitFest process is that, by reviewing patches, a submitter learns more about the code base. It is a community effort, he said, "and each of us can stand to improve our knowledge of what is fundamentally a complex system." Less-experienced reviewers are no problem, he said, and it is fair to expect reviewers to ask questions or request better comments and explanations if they find something unclear. David Rowley agreed, adding that the CommitFest offers a chance for reviewers to work side-by-side with patch submitters, confident that the submitters will have the time to reply (and perhaps iterate on the patch), since new development is on pause. "Even if a novice reviewer can only help a tiny bit, it might still make the difference between that patch making it to a suitable state in time or it getting bounced to the next commitfest or even the next release." Rowley said that he had started out as a novice in the PostgreSQL 8.4 cycle, and learned a great deal by reviewing patches and by being perfectly clear about what he did and did not understand. Other new reviewers, he said, would do well to be up front about the limits of their experience as well. Almost every free software project grapples with the fundamental problem of cultivating new users and developers into experienced contributors. PostgreSQL's patch-review policy for CommitFests is clearly part of its program to do such developer cultivation. To that end, it may not matter so much how the project notifies patch submitters that they are also on the hook for a patch review, just as long as the submitter participates. PostgreSQL gets better-reviewed code, and the patch reviewer learns something in the process. Seeing one's name of a list of people who still owe a review might be surprising to some, but judging by the responses on the mailing list, at least a handful of those listed got on the ball and signed up for a patch to review. Consequently, it is hard to argue that the list of names did not have a positive effect on the project. Furthermore, if there were any hurt feelings, Berkus subsequently took steps to mend them, asking the list for suggestions on how to give public kudos to patch reviewers. What is still up in the air is whether such kudos will take the form of public thanks, physical gifts, or something else altogether.
Brief items Quotes of the week
It’s easy for a misbehaving program to write garbage, but not to worry! we’re absolutely certain that garbage is consistently replicated across the cluster. Yeah, well done there.
— Andrew Cowie.
Wha? Is there anyone in the world who needs to sync their feeds between two different phones?
— Jamie
Zawinski, lamenting the feature set of Google Reader replacements.
Introducing DaalaDaala is a new video codec from Xiph.org; the project has posted a detailed explanation of how it works. "Daala tries for a larger leap forward— by first leaping sideways— to a new codec design and numerous novel coding techniques. In addition to the technical freedom of starting fresh, this new design consciously avoids most of the patent thicket surrounding mainstream block-DCT-based codecs. At its very core, for example, Daala is based on lapped transforms, not the traditional DCT."
PHP 5.5.0 releasedVersion 5.5.0 of the PHP language is out. New features include generator and coroutine support, a new finally keyword, a better password hashing API, a new opcode caching mechanism, and more.
Changes coming for systemd and control groupsLennart Poettering posted a message to the systemd-devel mailing list that outlines some changes coming. It refers to an earlier message that started describing differences coming in the kernel's control group (cgroup) implementation and their effects on systemd. Those cgroup changes were discussed in February 2012 and cgroup maintainer Tejun Heo decided to eventually drop multiple cgroup hierarchies in March 2012. The basic idea is that the single hierarchy will also have a single manager on the system and all changes to the cgroups will need to go through that manager. For systems that use it, systemd will become the manager of cgroups, so other tools that want to make changes will no longer follow the PaxControlGroups recommendations and will instead make D-Bus requests to systemd. Non-systemd systems will need to come up with their own manager and management interface once the cgroup changes are made in the kernel (for which the timeline is unclear). "In the long run there's only going to be a single kernel cgroup hierarchy, the per-controller hierarchies will go away. The single hierarchy will allow controllers to be individually enabled for each cgroup. The net effect is that the hierarchies the controllers see are not orthogonal anymore, they are always subtrees of the full single hierarchy."
osm-gps-map 1.0.0 releasedVersion 1.0.0 of the osm-gps-map library has been released. The library is a general-purpose mapping widget supporting OpenStreetMap and other online services, as well as GPS tracks. This is the first stable release in more than a year, and adds support for GTK+ 3.0.
Firefox 22 is now availableMozilla has released Firefox 22. Among the changes in this version are WebRTC support being enabled by default, major performance improvements to ASM.js, playback-rate adjustment for HTML5 audio and video, the addition of support for HTML5 <data> and <time> elements, social networking management and, best of all, a built-in font inspector.
LibreOffice 4.0.4 availableThe Document Foundation has announced LibreOffice 4.0.4, which is expected to be the final minor release before the announcement of LibreOffice 4.1.0, currently slated for late July. A number of improvements to interoperability with proprietary office suites land in 4.0.4, in addition to the usual host of bug- and regression-fixes.
Newsletters and articles Development newsletters from the past week
Sanders: A picture is worth a thousand wordsOn his blog, Vincent Sanders peers in on the process of rendering an image in a web browser. While conceptually simple, there is actually a fair amount of work that has to be done to get from the bits over the wire to the image on the screen. "In addition the image may well require tiling (for background gradients) and quite complex transforms (including rotation) thanks to CSS 3. Add in that javascript can alter the css style and hence the transform and you can imagine quite how complex the renderers can become."
Desperate times call for Tux Paint (opensource.com)Opensource.com has an interview with Tux Paint creator Bill Kendrick. Tux Paint is a drawing program targeted at children, but is used by adults, particularly teachers. "I didn't design Tux Paint to be a tool that's only used for teaching art, I designed it to be a tool like a piece of paper and pencil. Some schools use it to create narrated storybook videos. Other schools use it for counting and math. Ironically, these days, after being exposed to things like Mr. Pencil Saves Doodleburg on my son's Leapster Explorer (which, by the way, runs Linux!), I'm beginning to think Tux Paint needs some additional features to help teach concepts about art and color. Or at least a nicely done set of curricula."
California Says the Bitcoin Foundation Is a Money-Transferrer (Wired)Wired notes a potentially major complication for the Bitcoin Foundation: the California Department of Financial Institutions has decided that the foundation is in "the business of money transmission," which would subject it to regulatory scrutiny, and has sent it a cease-and-desist letter. For its part, the Bitcoin Foundation does not seem to feel that the letter is a source of trouble, but evidently other US states are similarly taking a close look at how Bitcoin operates.
Page editor: Nathan Willis |
|||||||||||||