Publishing code reviews

One of the engineering practices that’s served the JDK development team well for many years now is that of peer-based code reviews. In mainline JDK development at least one review is required for every code change, and two or more are needed as release milestones approach.

Reviews are most often performed with the help of a tool called “webrev,” which basically diffs two source trees and generates a pile of HTML that you can view in a browser (sample). The webrev tool was originally written by the Solaris team; lately it’s been revised and extended to support Mercurial and Subversion in addition to the old internal TeamWare SCM.

A lot of the e-mail that JDK developers read and write is, naturally, about code reviews. It’s not very convenient to attach webrevs to e-mail messages, or to read such attachments. People therefore typically either copy webrevs to a directory published by a private web server and send the URL around, or they copy them over to an NFS-exported filesystem and send a long pathname—and cross their fingers if a potential reviewer is far away network-wise.

The bug here, of course, is that these publication methods don’t make code reviews visible on the open web, and this is one reason that most JDK developers have thus far been reluctant to move more of their e-mail traffic onto the public OpenJDK mailing lists.

So: How can we solve this problem in a way that works for all OpenJDK contributors, whether or not they work for Sun?

We could build something slick and fancy, like Google’s Mondrian, but I think it’s relatively more important to get something up quickly and work on improving it later.

A more expedient solution would be to construct something like the OpenSolaris code-review site, where contributors can use rsync, scp, and sftp (all via SSH) to upload webrevs to temporary storage on a public server. SSH keys will ultimately be required in order to push changesets into the public Mercurial repositories, so contributors will need to create and register SSH keys anyway, and the code-review site can easily leverage the same underlying infrastructure.

Comments? Alternatives?

(Please direct followups to the discuss@openjdk.java.net mailing list. If you haven’t already subscribed to that list then please do so first, otherwise your message will be discarded as spam.)

Comments:

While crucible doesn't support Mercurial, I bet that support could be added for a reasonable amount of time and money.

http://www.atlassian.com/software/crucible/

Posted by Curt Cox on October 11, 2007 at 06:07 AM PDT #

Have you seen Review Board? It is similar to Google's Mondrian, and would likely be a good place to start. It comes out of VMware:

http://www.review-board.org/

Evan Jones

Posted by Evan Jones on October 11, 2007 at 10:09 AM PDT #

Curt: I'm sure that Mercurial support could be added to Crucible, but since OpenJDK is an open-source community we'd generally prefer to stick to open-source tools, which Crucible is not.

Evan: Thanks for the pointer to Review Board. I remember hearing about it when it was launched, but had since forgotten. We'll definitely check it out.

Posted by Mark Reinhold on October 11, 2007 at 01:17 PM PDT #

> ..to get something up quickly and work on improving it later.

You are fooling yourself if you believe that will actually work. :)

Posted by Mikael Grev on October 12, 2007 at 04:23 AM PDT #

OK, that wasn't very constructive of me.

How about this:

Set up a snazzy tool where everyone can review JDK developers' code though making comments and suggestions. Other reviewers, and Sun, can review how 'usable' certain reviewers' comments are. A good reviewer get a lot of 'points' and his opinions with weigh more. The best reviewers can get paid; after all they do free up time for the JDK developers and thus add value.

This would be a good way to free JDK developer of these boring tasks. It would also be a good way in for good external developers. They can prove themselves though the system.

Sun will gain and so will the reviewers.

Cheers,
Mikael

Posted by Mikael Grev on October 12, 2007 at 04:32 AM PDT #

Post a Comment:
Comments are closed for this entry.
About

This blog has moved to http://mreinhold.org/blog. <script>var p = window.location.pathname.split('/'); var n = p[p.length - 1].replace(/_/g,'-'); if (n != "301") window.location = "http://mreinhold.org/blog/" + n;</script>

Search

Archives
« April 2014
SunMonTueWedThuFriSat
  
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
   
       
Today
News

No bookmarks in folder

Blogroll

No bookmarks in folder

Feeds
RSS Atom