On Mon, Mar 28, 2011 at 4:31 PM, Rob Lanphier robla@wikimedia.org wrote:
Right now, we have a system whereby junior developers get to commit whatever they want, whenever they want. Under the system you outline, the only remedy we have to the problem of falling behind is to throw more senior developer time at the problem, no matter how ill-advised or low-priority the changes the junior developers are making. Taken to an extreme, this means that junior developers maintain complete control over the direction of MediaWiki, with the senior developers there purely in a subservient role of approving/rejecting code as it comes in.
If you let months and months of code review pile up, then yes, people will have to do code review full-time to catch up. But if you review all code as it comes in, and you have a sensibly large pool of reviewers, no one person will have to spend more than a moderate amount of their time (much less than half) reviewing code. You don't have to limit review to only really senior people -- most developers should be competent enough to review patches to code they wrote, for instance.
Reviewing code normally takes *much* less time than writing it. If reviewing code takes a fifth the time that writing it does, then any volunteer-submitted code is available for Wikimedia use at one-fifth the regular cost. Even if it's something Wikimedia wouldn't normally have devoted resources to develop, it's likely to at least be worth the resources to review. Code that's entirely useless to Wikimedia is usually going to be code that doesn't run on Wikimedia sites, which doesn't need to be reviewed at all. Plus you get the benefit of using volunteers as a sort unpaid interns, whom you can later decide to hire.
Really, though, this is a matter of fact, not opinion. What reason do you have to believe that code review will actually take up so much time? Lots of projects accept volunteer contributions and ensure they get promptly reviewed, like Mozilla, WebKit, and Chromium. It works well for them; why not for us? It's not like Google is interested in paying expert developers to waste all their time reviewing useless contributions to Chromium, or likewise for any similar project. For instance, as one data point, how many developer-hours were needed to clear each week of commit backlog? It must be a lot less than forty times the number of developers assigned to it, or you wouldn't have been able to clear the backlog at all.
What comes of this system should be obvious: senior developer burnout. If only reward we offer for becoming an experienced developer is less interesting work with less power over day-to-day work, we're not going to attract and retain people in senior positions.
If particular developers dislike code review, they can be given less code review to do and more coding. It's also not necessary to assign only the most senior developers to code review. The workload should be distributed in whatever way makes sense.
But really, if the problem is that it's the senior developers who don't want to review code, then I agree, that's a problem. Is that the case? The post by Tim Starling that I referred to suggested that he felt experienced developers should be assigned to do more code review than they are, and that it was management that wanted to de-prioritize it rather than the reviewers themselves. Roan and Brion have also both stated pretty clearly that they want to see a return to prompt review and regular deployment. That's three senior developers right there. Which are the ones you're afraid will burn out?
On Mon, Mar 28, 2011 at 6:28 PM, Trevor Parscal tparscal@wikimedia.org wrote:
I must whole-heartedly agree with RobLa here. For quite some time we had 2 reviewers, then just 1. Naturally unreviewed revisions started to pile up, so we assigned more of our developers to perform code review. This got us out from under a ridiculous backlog and get 1.17 out into the wild, but in the process very little else got done.
It's important that we all work together on features, not just the junior people only to get a "pass" or "fail" from the senior people.
Of course very little will get done if you let a huge backlog accumulate. If you review code on a regular, sustainable basis, there will be plenty of time to do other things, especially if reviewers are judicious in rejecting patches that they think are too difficult to review for the benefit. Put another way, the fact that paid developers could clear out a year's backlog working more or less full-time for much less than a year demonstrates that to keep up with regular review would require much less than full-time work.