On 31.05.2011, 22:41 Rob wrote: Social problems are hard to fix using technical means. What will happen if someone approves a revision that later turns out to be defective, off with their head? "OMG my commit will be wasted in 20 minutes! Heeelponeone" is not the best scenario for clear thinking. Time pressure might result in people reviewing stuff they're less familiar with, and reviews being done in a haste, so CR quality will suffer. Although I'm not a biggest fan of rushing towards DVCS, the distributed heavily-branched model where mainline is supported only through merges from committed-then-reviewed branches looks far more superior to me. This way, we will also be able to push stuff to WMF more often as there will be no unreviwewed or unfinished code in trunk. -- Best regards, Max Semenik ([[User:MaxSem]])

Rob Lanphier wrote: <snip> Russell N. Nelson - rnnelson wrote: Perhaps a combination is possible, a bit like Brion did in the old days: * A revision has to be reviewed within reasonble time (time upto it still allows reverting without pain, ie. hours or days. Not weeks! No more than, say, 3 days) * The people who do code review, which should always start at the back, will see soon enough if there's a revision stuck in the process (ie. not being reviewed / passed to someone else) at that point someone has to go bold and revert it. Nothing personal: If nobody understood your commit, either splits it up and document it better – or poke whoever is capable of reviewing your code and make sure he or she does so in time. We are responsible as a team (nothign like primary school hand-in-hand pairs of buddies, but you get the idea..). * A fixme should not maintain it's status for more than 24 hours. Not fixed within 24 hours ? Revert, no questions asked! * Once a week, on Monday, we make sure anything from after Friday noon (remember, 3 days!) is unreviewed or fixme'ed. They shall be reverted or sorted out the same day (some gnomes may want to do this over the weekend). * Tuesday anything last week should no longer be on our minds. We've moved on! "Tomorrow" (wednesday) is already the 3rd day of the week. Aside from the in-svn proccess, some points I learned from other committers and just now via IRC: * Unless the committer is subscribed to mediawiki-cvs or keeps a tight eye on Special:Code, one should have wiki-account linked in the CodeReview extension and e-mail notifications on (so you know about questions/comments., status changed and follow-ups as soon as possible) * the "todo" tag (or soon-to-be-introduced status "needsmore" or "incomplete")[1] does not mean "in another life time". * If you don't know where to start... http://etherpad.wikimedia.org/CodeReviewTracker -- Krinkle [1] https://bugzilla.wikimedia.org/28274

On Tue, May 31, 2011 at 12:37 PM, Max Semenik &lt;maxsem.wiki(a)gmail.com&gt; wrote: This isn't a technical solution, but a social one to force us to make sure that things that haven't been looked over GET looked at. Consider it to already be our hypothetical model -- it's just not been enforced as consistently as it's meant to. Bad code -> fixed or reverted ASAP. Unseen code -> assumed bad code, so look at it. I'd say better to do that continuously and confirm that things are being reviewed *and* run on automated tests *and* run against humans on actual-production-use beta servers *and* in actual production use within a reasonable time frame, than to let review slide for 10 months and rush it all right before a giant release. That IMO is what really hurts code review -- removing it from the context of code *creation*, and removing the iterative loop between writing, debugging, testing, debugging, deploying, debugging, fixing, and debugging code. I tend to agree that a more active 'pull good fixes & features in from feeder branches' model can be helpful, for instance in allowing individual modules, extensions, etc to iterate faster in a shared-development environment without forcing the changes to trunk/mainlines's production deployment & release channels until they're ready. But these need to be combined with really active, consistent management; just like keeping the deployment & release branches clean, working, and up to date consistently requires a lot of attention -- when attention gets distracted, the conveyor belt falls apart and fixes & features stop reaching the public until the next giant whole-release push, which turns into a circus. -- brion

"Russell N. Nelson - rnnelson" &lt;rnnelson(a)clarkson.edu&gt; wrote in message news:777BC8A5A78CC048821DBFBF13A25D39208F1141@EXCH01.ad.clarkson.edu... By far the most likely outcome of this is that in the two months following its introduction, 95% of all commits are reverted, because the people who are supposed to be reviewing them don't spend any more time than usual reviewing them. If I make a change to the preprocessor, HipHop, Sanitizer, SecurePoll, passwords, tokens, or any of a number of other things, I'm going to need Tim to review them. I don't begrudge Tim the thousand-and-one other things he has to do besides review my code within 72 hours. Does that mean that no one should make *any* changes to *any* of those areas? More dangerously still, does that mean that **only people who can persuade Tim to review** be allowed to make changes to those areas? I know what the answers to those questions are *supposed* to be, that's not the point. The point is **what actually happens**. Every way of phrasing or describing the problem with MW CR can be boiled down to one simple equation: "not enough qualified people are not spending enough time doing Code Review (until a mad rush before release) to match the amount of code being committed". Any attempt at a solution which does not change that fundamental equation is doomed to failure. There are at least six things that could be changed ("enough people", "qualified people", "enough time", "Code Review", "match[ing]", "being committed"); we almost certainly need to change more than one. The most likely outcome of this particular suggestion is simply to radically reduce the amount of code being committed. I'm not sure that that's the best way to deal with the problem. --HM

"The Tiki community is a heavy user of SVN. While many open source communities have a send-a-patch-and-someone-else-will-commit approach, in Tiki, we encourage everyone to commit directly to the source code. Think of it as applying the Wiki Way to software development. Over 220 people have done it so far and it works very well." http://dev.tiki.org/How+to+get+commit+access I don't know if this is feasible in this community - but wanted to put it out is as a possible direction. Best, Mark On 31May2011, at 4:52 PM, Chad wrote:

On 01/06/11 01:49, Happy-melon wrote: I fully support the idea of reducing the amount of code. My main issue is where to send the code: - bugzilla as an attached patch ? - github for the social effect ? I even volunteer to get my commit right disabled to experiment this :) -- Ashar Voultoiz

"Chad" &lt;innocentkiller(a)gmail.com&gt; wrote in message news:BANLkTinRbx45WSG6YycLWpZpO7MJb3RwWA@mail.gmail.com... That's a worthy goal, but one that's orthogonal to Code Review. Every single person on this list will have committed some unreviewed code to some repository at some time; the fewer times you've done it, the more likely you are to have crashed the cluster the times you did. People doing some unquantifiably greater amount of testing doesn't mean we can spend any less time on CR per revision. Automated testing, regression testing, other well-defined infrastructures (I think Ryan's Nova Stack is going to be of huge benefit in this respect) *do* save CR time *because reviewers know exactly what has been tested*. A policy like "every bugfix must include regression tests" would definitely improve things in that area. Of course, it's undeniable that more testing would lead to fewer broken commits, and that that's a Good Thing. If we implement processes which set a higher bar for commits 'sticking' in the repository, whether that's pre-commit review, a branch/integrate model, post-commit countdown, whatever; people will rise to that level. --HM

On Wed, Jun 1, 2011 at 11:23 AM, Roan Kattouw &lt;roan.kattouw(a)gmail.com&gt; wrote: I don't think "revert in 72 hours if its unreviewed" is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I *do* think we should enforce a 48hr "revert if broken" rule. If you can't be bothered to clean up your breakages in within 48 hours of putting your original patch in, it must not have been very important. -Chad

On 01.06.2011, 7:59 Mark wrote: Our commit access approval is quite liberal already, however it's not completely instantaneous - people still need to contribute a few patches before being allowed to work on core, due to the importance of security in MediaWiki as it's being used on a top 5 website. We already have 315 commiters (a few of accounts are dupes though). The problem is not about attracting more contributors, it's about reviewing and deploying their code. -- Best regards, Max Semenik ([[User:MaxSem]])

That's because our phpunit setup is incredibly fragile. I'm trying to fix it. -Chad On Jun 1, 2011 12:09 PM, "Max Semenik" &lt;maxsem.wiki(a)gmail.com&gt; wrote: http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw>gt;. Possible culprits: aashrh/r89027 /r89028 /r89029 nbiabriket/r89035 krenkli/r89036 eryed/r89037 /r89038 eixal/r89039 /r89040 /r89041 /r89043 /r89044 /r89047 /r89049 /r89051 /r89061 /r89062 /r89063 /r89070 /r89071 /r89072 yaor_mdn/r89074 /r89075 /r89076 osnenl/r89079 /r89082 /r89083 /r89084 /r89085 /r89086 /r89087 eom-ylnphap/r89088 jedht/r89094 /r89099 /r89 /r89115 /r89116 /r89117 /r89118 /r89119 iwkiunta/r89120 /r89122 /r89123 onraa/r89128 /r89129 /r89134 /r89138 ^nmdoe/r89144 /r89145 /r89149 /r89150 algsinrtt/r89166 /r89176 /r89179 /r89180 sxemma/r89181 /r89182 /r89186 ptolsaiedn/r89191 /r89197 /r89204 /r89205 /r89206 /r89207 /r89208 /r89218 wadreoujdene/r89219 /r89223 /r89224 rtemo/r89225 /r89226 /r89227 /r89228 /r89230 /r89241 oribn/r89243 /r89244 /r89 awnurotkaot/r89258 /r89260 rnabsedi/r89261 /r89262 /r89263

On Wed, Jun 1, 2011 at 8:23 AM, Roan Kattouw &lt;roan.kattouw(a)gmail.com&gt; wrote: I think rather than us being all screwed, this is a situation where we *don't actually have to* commit that change to trunk right at that exact time. Unless the change needs to be deployed immediately -- in which case you really should have someone else looking it over to review it IMMEDIATELY -- it can probably wait until reviewers are available. And if it does get reverted before the reviewer gets to it -- what's wrong with that? It'll get reviewed and go back in later. Part of the reason we went so hog-wild with handing out commit access for a long time is because we've never *been* as good as we want about following up with patches submitting through the bug trackers and mailing lists; sometimes things get reviewed and pushed through, but often they just get forgotten. Letting people commit directly meant that things didn't get forgotten -- they were right there in the code now -- with the caveat that if there turned out to be problems, we would have to go through and take them back out. It's a trade-off we chose to make at the time, and it's a trade-off we can revisit if we change the underlying conditions. -- brion

On Thu, Jun 2, 2011 at 1:28 AM, Chad &lt;innocentkiller(a)gmail.com&gt; wrote: I'm really not a fan of drop-dead deadlines in the one to two day range in general. I occasionally have periods of about two or three days when I don't have access to a computer (or time to use one). I think that if we actually want pre-commit review (which I don't have a problem with), we should have pre-commit review instead of excessive reversion. Reverts make the revision log hard to follow, feel like a slap in the face to many developers (especially new ones, who this policy is supposed to be attracting!), and of course give us lots of merge conflicts and what not. I think it would combine with commits of code that's broken in the first place to exacerbate the current situation where a single change can have up to ten associated revisions where people fix little things, revert, unrevert, and generally make things difficult to review and follow. -- Andrew Garrett Wikimedia Foundation agarrett(a)wikimedia.org

Dear friends, please - can the discussion under this subject be continued in a forum ? Tom

On 01.06.2011, 23:17 Thomas wrote: What forum? wikitech-l is a list designed for discussions related to MediaWiki development and Wikimedia technical stuff. Exactly what we're discussing right now. -- Best regards, Max Semenik ([[User:MaxSem]])

On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor &lt;Simetrical+wikilist(a)gmail.com [snip] [snip] "This isn't ready for core yet -- code looks a bit dense and we don't understand some of what it's doing yet. Can you help explain why you coded it this way and add some test cases?" It's usually better if you can do this at a pre-commit stage: when reviewing a patch on bugzilla, when looking over a merge request from a branch, etc. But it's ok to pull something back out of trunk to be reworked, too. -- brion

On Wed, Jun 1, 2011 at 4:06 PM, Brion Vibber &lt;brion(a)pobox.com&gt; wrote: IMO, this is totally fine as a reason to reject a commit. It gives clear reasons why it's being rejected and suggests how to fix them. Another good reason, which I've given in the past to at least one thing I was asked to review on Bugzilla, is "This is doing too many things at once -- break it up into a bunch of smaller patches so I can review them individually." But just letting the patches languish with no reason is really bad, and reverting them with no reason once they were already committed is psychologically even worse (even though practically it's similar).

Max Semenik wrote: Actually, we have made security releases that *broke* tests. (ie. tests got outdated) The problem is that we don't have a proper architecture for testing everything. For instance, I would have liked to add a test for 'stub color in contribution lists' when fixing it. But we have no tests able to check that. Perhaps we could track it with Selenium or storing the html output files, but we would need a more advanced testing fake db.

On Wed, Jun 1, 2011 at 4:41 PM, Happy-melon &lt;happy-melon(a)live.com&gt; wrote: This is one of the reasons I tend to advocate shorter development/review/deployment cycles. By keeping the cycle short, we can help build up regular habits: run through some reviews every couple days. Do a deployment update *every* week. If you don't think your code will be working within that time, either work on it outside of trunk or break it up into pieces that won't interfere with other code. With a long cycle, review gets pushed aside until it's no longer habit, and gets lost. -- brion

On 06/01/2011 08:28 AM, Chad wrote: I think a revert on sight, if broken is fair ... you can always re-add it in after you fix it ... if its a 'works diffrently than expected' type issue / not perfectly following coding conventions a 48hr window to make progress ( during the work week ) sounds reasonable. --michael

I've done release engineering before. It is my considered opinion that one person needs to be on point for each release. It's possible that that person could rotate in and out, but one person needs to be running through the checklist of "things that need to be done". ________________________________________ From: wikitech-l-bounces(a)lists.wikimedia.org [wikitech-l-bounces(a)lists.wikimedia.org] on behalf of Erik Moeller [erik(a)wikimedia.org] Whether that would get us to a healthier balance remains to be seen, but I think there's pretty broad agreement that adding more support to the review/deployment/release process is a necessary precondition for any other process changes like moving towards pre-commit review. Clearly what's been said in this thread is true -- there are lots of things that can be done to reduce our technical debt and make it easier to accommodate and manage new changes, but without added dedicated capacity, the train won't move at a speed that we're happy with. We can't change that overnight (because we need to figure out the right rhythm and the right processes together), but we will change it. Erik -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

On Wed, Jun 1, 2011 at 9:09 PM, Russell N. Nelson - rnnelson &lt;rnnelson(a)clarkson.edu&gt; wrote: I've mentioned on several occasions in the past that we should try this. If each branch (REL1_17, 1.17wmf1, REL1_18) had someone to babysit it, it would lead to less turnaround for things to get merged to various branches because someone would be the "go-to" person for getting a trunk change into the branch in question. Prior to the 1.17beta1 release, a *huge* number of revs got queued up in CR because Roan stopped merging and then I finally noticed the list. As a result, I ended up having to sort through quite a few merges (in SVN, no less...) that were messy because we'd let too much time pass before merging them to where they need to be. Having someone who's job it is to keep an eye on their branch would ensure timely merges as well as keeping the branches in better shape IMO. -Chad

On Thu, Jun 2, 2011 at 2:58 AM, Erik Moeller &lt;erik(a)wikimedia.org&gt; wrote: I don't think our tech dept is big enough to be able to do rotating teams. There are quite a few tasks that are the domain of 3 or fewer people, so some of those people need to be involved with service work all the time and can't really be rotated out of it. I like the 20% model as well. It goes a little bit further than my suggestion, which was 20, 25 or 33% model for certain senior devs, but if you're volunteering to give me more than what I asked for that's even better :) . For numerous reasons, I think getting /everyone/ involved with service work is a great idea. There are two potential pitfalls that I think we should be aware of: * Since 20% == one day for full-time employees, they will probably tend to do all of their tasks on one day. Which is fine by me, as long as not everyone picks the same day :) (people might want to use their WFH day for this, and for most people that's Wednesday). So we should take care to spread this around a bit * The relative prioritization of service tasks should be focused on "what can you, as a staff member with your skills, do that a random volunteer dev couldn't?". This means review, deployment and shell bugs should be prioritized over bug triaging and fixing, because the former category requires specific skills or permissions that a lot of people don't have. There are more people out there that can triage and fix bugs, so while WMF staff should definitely continue doing that (some bugs are particularly mysterious and need a highly experienced dev to look at them) it shouldn't be at the expense of things like CR Related to resource allocation, and in response to Russ and Chad's comments, I agree that it would be good to have someone take the lead on releases and CR and such. Historically, this person has either been RobLa (general engineering EPM) or Mark H (Bugmeister), and they've only been coordinating as opposed to actively keeping an eye on review queues, merging revs, reviewing things that get left by the wayside, and so on. Maybe it's a good idea to designate one person as the leader and 'default' person (i.e. if you don't know who to assign something to, assign it to them and they'll either do it or get someone else to) for CR and branch management. That person should not be subject to the 20% rule, but should be spending as much time on it as is needed to get stuff done. This position could probably be rotated within the gen eng group. Roan Kattouw (Catrope)

Chad wrote: +1

On Fri, Jun 3, 2011 at 3:33 PM, Rob Lanphier &lt;robla(a)wikimedia.org&gt; wrote: I accept this challenge! My 20% is committed for next week... are y'all with us? :D -- brion

On 04/06/11 00:33, Rob Lanphier wrote: It is a thin red line. Lets just pretend we are in the Crimean Peninsula. -- Ashar Voultoiz Would let users figures it out by searching their favourite encyclopaedia.

On Sun, Jun 5, 2011 at 9:13 PM, MZMcBride &lt;z(a)mzmcbride.com&gt; wrote: I think one thing to do is to help triage the 1.17 blockers: https://bugzilla.wikimedia.org/show_bug.cgi?id=26676 It appears a few more were added to the list over the weekend, including bug 24415 (tracking bug for Resource Loader 1.0), for which there are many blockers. If these bugs are all truly blockers, then we'll have to pull people in from code review to fix those bugs. If we can wait until 1.18 or later, then we'll have the luxury of holding out for volunteer help. Rob

Erik Moeller wrote: This looks great! Both your post and Brion's yesterday have been spot-on. I think with a few people doing shell bugs every week (as an example), the backlog will be killed in no time. "Shell" bugs are generally some of the most user-facing issues, so reducing time between filing and fulfillment on those will go a long way toward keeping users happy. There are also structural improvements that can be made (such as getting a "Configure" extension working on Wikimedia wikis) that could eliminate the need for sysadmin intervention a lot of the time. I'm not sure if working on projects that like would fall within the 20% rule as written, but it's something to think about. The only other point from your list that I think maybe could possibly be made explicit is a focus on the sister projects. It's pretty clear that during the 80%, Wikipedia is the central focus. I've been doing some work on Wikisource lately and without looking very hard, it's very easy to see how some support structures on the sister sites (API support, rewrites of some of these extensions, moving code from JavaScript to PHP, etc.) could really enable third-party developers. Overall, this seems like a really positive step in the right direction. :-) MZMcBride