On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar neilk@wikimedia.org wrote:
Are we all in deadlock or something? Are the users who can push waiting from some proposals/work from the rest of the community?
We had a hallway conversation about this just now (Neil, Trevor, Brion and I, and then just Brion and I), which I think was pretty useful. Here's where we went with it:
1. We rehashed the pre-commit review proposal that Neil suggested a few months ago, and agreed that pre-commit would be helpful in keeping the backlog down 2. Given our current tools/process, we agreed that insisting on pre-commit review would be a pain in the butt. 3. Brion and I further discussed review process, trying to come up with a system that give us the benefits of pre-commit review, without actually switching to pre-commit review
Here's where things I think got interesting. Brion pointed out that in ye olden days, he was much more aggressive about reverting things he didn't understand. I pointed out that, as we broaden the pool of committers, "I don't understand"-based reversions lead to a lot of ugliness, since very few people claim to have a broad understanding of the system and therefore an expectation of understanding every change. Most reviewers, faced with a commit they don't understand, will leave it for others to comment on. There's been a lot of unnecessary drama and churn over reversions because of misunderstandings about what a reversion means.
So, there's a number of possible solutions to this problem. These are independent suggestions, but any of these might help: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code.
I coulda swore there are other ideas that came out of that conversation, but alas, I wasn't taking notes. Anyway, I'm sure they'll come up in this thread.
Rob
Robla writes: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process.
Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review.
And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says.
On Tue, May 31, 2011 at 12:09 PM, Russell N. Nelson - rnnelson < rnnelson@clarkson.edu> wrote:
Robla writes:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process.
Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code.
For the same reason my position is that a "minimum time before revert" is not desireable -- if anything is ever found to be problematic, the correct thing is to remove it immediately.
Robla counters this with the observation that when there are multiple people handling code review, simply being *uncertain* about whether something needs a revert or not is better grounds for *passing it on* to someone else to look at than to immediately revert.
This should usually mean a few minutes on IRC, however, not just a 72-hour silence without feedback!
What really worries me though isn't the "somebody else built a patch on this within 72 hours, now it's slightly awkward to revert" case but the "other people built 100 patches on this in the last 10 months, now it's effing impossible to revert".
Ultimately, we need to remember what reverting a commit means:
** Reverting means we're playing it safe: we are taking a machine in unknown condition out of the line of fire and replacing it with a machine in known-good condition, so the unknown one can be reexamined safely in isolation while the whole assembly line keeps moving. **
It's not a moral judgment against you when someone marks code as fixme or reverts it; it's simple workplace safety.
-- brion
Brion Vibber wrote:
Ultimately, we need to remember what reverting a commit means:
** Reverting means we're playing it safe: we are taking a machine in unknown condition out of the line of fire and replacing it with a machine in known-good condition, so the unknown one can be reexamined safely in isolation while the whole assembly line keeps moving. **
It's not a moral judgment against you when someone marks code as fixme or reverts it; it's simple workplace safety.
Often wiki workflow follows code development workflow, but in this case, code development workflow can take a lesson from the wiki: http://en.wikipedia.org/wiki/Wikipedia:BOLD,_revert,_discuss_cycle. :-)
MZMcBride
"Russell N. Nelson - rnnelson" rnnelson@clarkson.edu wrote in message news:777BC8A5A78CC048821DBFBF13A25D39208F1141@EXCH01.ad.clarkson.edu...
Robla writes:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process.
Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review.
And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says.
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
On Tue, May 31, 2011 at 7:49 PM, Happy-melon happy-melon@live.com wrote:
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".
Maybe people shouldn't commit untested code so often.
I'm not joking.
-Chad
"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 Tue, May 31, 2011 at 7:49 PM, Happy-melon happy-melon@live.com wrote:
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".
Maybe people shouldn't commit untested code so often.
I'm not joking.
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 01.06.2011, 7:59 Mark wrote:
"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."
I don't know if this is feasible in this community - but wanted to put it out is as a possible direction.
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.
"Chad" innocentkiller@gmail.com wrote in message news:BANLkTinRbx45WSG6YycLWpZpO7MJb3RwWA@mail.gmail.com...
On Tue, May 31, 2011 at 7:49 PM, Happy-melon happy-melon@live.com wrote:
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".
Maybe people shouldn't commit untested code so often.
I'm not joking.
-Chad
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 01/06/11 01:49, Happy-melon wrote:
The most likely outcome of this particular suggestion is simply to radically reduce the amount of code being committed.
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 :)
On Wed, Jun 1, 2011 at 1:49 AM, Happy-melon happy-melon@live.com wrote:
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**.
This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed.
So yes, before we implement this we need to 1) ensure that reviewers spend enough time reviewing and 2) figure out what to do with one-reviewer situations.
Roan Kattouw (Catrope)
On Wed, Jun 1, 2011 at 11:23 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
On Wed, Jun 1, 2011 at 1:49 AM, Happy-melon happy-melon@live.com wrote:
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**.
This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed.
So yes, before we implement this we need to 1) ensure that reviewers spend enough time reviewing and 2) figure out what to do with one-reviewer situations.
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 Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkiller@gmail.com 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 also don't really like it. But I do think we should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever.
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.
+1. Serious breakage should be reverted on sight, however.
Roan Kattouw (Catrope)
On 01.06.2011, 20:00 Roan wrote:
On Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkiller@gmail.com 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 also don't really like it. But I do think we should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever.
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.
+1. Serious breakage should be reverted on sight, however.
+1. "Serious" should include "breaking the tests" though - otherwise we'll remain with our current situation when tests are broken in multiple places and nobody knows who broke what:
[19:39:08] <codurr> Something broke. See http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw. 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 [19:39:08] <codurr> /r89108 /r89110 /r89111 /r89112 /r89113 /r89114 /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 [19:39:08] <codurr> kfoelokwrays/r89250 /r89251 /r89253 /r89254 awnurotkaot/r89258 /r89260 rnabsedi/r89261 /r89262 /r89263
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" maxsem.wiki@gmail.com wrote:
On 01.06.2011, 20:00 Roan wrote:
On Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkiller@gmail.com 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 also don't really like it. But I do think we should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever.
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.
+1. Serious breakage should be reverted on sight, however.
+1. "Serious" should include "breaking the tests" though - otherwise we'll remain with our current situation when tests are broken in multiple places and nobody knows who broke what:
[19:39:08] <codurr> Something broke. See <
http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw%3E. 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
[19:39:08] <codurr> /r89108 /r89110 /r89111 /r89112 /r89113 /r89114
/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
[19:39:08] <codurr> kfoelokwrays/r89250 /r89251 /r89253 /r89254
awnurotkaot/r89258 /r89260 rnabsedi/r89261 /r89262 /r89263
-- Best regards, Max Semenik ([[User:MaxSem]])
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Jun 1, 2011 at 8:28 AM, Chad innocentkiller@gmail.com wrote:
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.
Officially speaking, since forever we have had an "it's acceptable to revert on sight if broken" rule.
If you encounter something broken, correct responses include: * revert it immediately to a known good state; send an explanation to the author about what's wrong (usually a CR comment and reopening a 'fixed' bugzilla entry) * figure out the problem and fix it; send follow-up explanation to the author (usually a CR comment and a Bugzilla comment if it's a followup on a bug fix) * take a stab at the problem and recommend a solution to someone else who will fix it straight away; including a note on CR or Bugzilla summarizing your IRC discussion is wise in case it ends up getting dropped * tag it fixme and ask someone more familiar with the stuff than you to handle it
-- brion
On Thu, Jun 2, 2011 at 1:28 AM, Chad innocentkiller@gmail.com 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.
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.
+1
I believe the way forward involves using pre-commit review, requiring test coverage to pass review, and developers working in branches at all times. SVN may be a pita when it comes to branches, but that's a solvable problem.
- Trevor
On Wed, Jun 1, 2011 at 10:52 AM, Andrew Garrett agarrett@wikimedia.orgwrote:
On Thu, Jun 2, 2011 at 1:28 AM, Chad innocentkiller@gmail.com 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.
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@wikimedia.org
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Dear friends,
please - can the discussion under this subject be continued in a forum ?
Tom
On Wed, Jun 1, 2011 at 3:17 PM, Thomas Gries mail@tgries.de wrote:
Dear friends,
please - can the discussion under this subject be continued in a forum ?
What forum? I can't think of any place more suitable than this list.
-Chad
On 01.06.2011, 23:17 Thomas wrote:
Dear friends,
please - can the discussion under this subject be continued in a forum ?
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.
On Wed, Jun 1, 2011 at 3:17 PM, Thomas Gries mail@tgries.de wrote:
Dear friends,
please - can the discussion under this subject be continued in a forum ?
Tom
Tom, if you might feel more comfortable following the discussion via a web interface, try
news://news.gmane.org/gmane.science.linguistics.wikipedia.technical
or
http://lists.wikimedia.org/pipermail/wikitech-l/2011-June/thread.html
On 06/01/2011 08:28 AM, Chad 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
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
On Wed, Jun 1, 2011 at 8:31 PM, Michael Dale mdale@wikimedia.org wrote:
On 06/01/2011 08:28 AM, Chad 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
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.
As long as we can agree that committing big things on a Friday is bad, because you might leave trunk broken all weekend :)
-Chad
On Wed, Jun 1, 2011 at 8:23 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed.
We could pick a different window, but I don't see how it can be much longer if we're also pursuing much more frequent deployments. If we're deploying every week, and something hasn't been reviewed at deploy time, the reviewer can either rush a review of code they may or may not really understand, or revert.
I think using the word "screwed" here belies the fundamental social problem we have with respect to reverts. As Brion has pointed out over and over, being reverted is not the end of the world. It doesn't mean "this code must die", it means "this code isn't ready for trunk yet". It still is in the repository and is available for comment and debate, and recommitting the code can happen a week or two later.
The genesis of the 72-hour idea was our discussion about what is stopping us from pre-commit review. The set of us in the office discussing this felt it was pretty much just a tools issue; from a policy point of view, we think it makes sense. Given that the 72-hour rule is less severe than pre-commit review, do you believe that our original premise is flawed (i.e. requiring pre-commit review is a bad idea)?
Rob
On Wed, Jun 1, 2011 at 12:38 PM, Rob Lanphier robla@wikimedia.org wrote:
The genesis of the 72-hour idea was our discussion about what is stopping us from pre-commit review. The set of us in the office discussing this felt it was pretty much just a tools issue; from a policy point of view, we think it makes sense. Given that the 72-hour rule is less severe than pre-commit review, do you believe that our original premise is flawed (i.e. requiring pre-commit review is a bad idea)?
The problem isn't reverting commits that are bad, it's reverting commits solely because they haven't been reviewed. In a pre-commit review system, the equivalent to the proposed 72-hour rule would be letting unreviewed code languish without comment. Both of these are bad things.
The point is, people's code should only be rejected with some specific reason that either a) lets them fix it and resubmit, or b) tells them definitively that it's not wanted and they shouldn't waste further effort or hope on the project. Whether you're using review-then-commit or commit-then-review, one of the most demoralizing things you can do to a volunteer's contributions is say "nobody cares enough to provide any feedback on your code, so we're just going to reject it by default".
Of course, all this requires time spent reviewing. It used to be that we had enough time in practice, even though volunteers greatly outnumbered paid staff. Now we seem to have a comparable number of paid staff and volunteers, so it seems clear that Wikimedia could find enough time if it wanted. But apparently Wikimedia's tech leadership feels it's more important to work on projects Wikimedia has a specific interest in, than to ensure that volunteer code is reviewed and deployed speedily and reliably. So the latter doesn't happen.
On Wed, Jun 1, 2011 at 2:01 PM, Trevor Parscal tparscal@wikimedia.org wrote:
I believe the way forward involves using pre-commit review, requiring test coverage to pass review, and developers working in branches at all times. SVN may be a pita when it comes to branches, but that's a solvable problem.
So then what happens if volunteers' contributions aren't reviewed promptly? In commit-then-review, they sit in trunk for months, by which point they're practically impossible to revert, so someone has to review them no matter what. In review-then-commit, they just never get committed, so they bitrot and are never used. The latter is incomparably more damaging to volunteers' willingness to participate.
On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor <Simetrical+wikilist@gmail.com
wrote:
[snip]
The point is, people's code should only be rejected with some specific reason that either a) lets them fix it and resubmit, or b) tells them definitively that it's not wanted and they shouldn't waste further effort or hope on the project.
[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 brion@pobox.com wrote:
"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?"
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).
On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor <Simetrical+wikilist@gmail.com
wrote:
So then what happens if volunteers' contributions aren't reviewed promptly?
Indeed, this is why we need to demonstrate that we can actually push code through the system on a consistent basis... until we can, nobody seems willing to trust pre-commit review.
-- brion
"Brion Vibber" brion@pobox.com wrote in message news:BANLkTikTz=V77o8vYMxbA91dj=PNNbvO=w@mail.gmail.com...
On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor <Simetrical+wikilist@gmail.com
wrote:
So then what happens if volunteers' contributions aren't reviewed promptly?
Indeed, this is why we need to demonstrate that we can actually push code through the system on a consistent basis... until we can, nobody seems willing to trust pre-commit review.
-- brion
+1. Pre-commit-review, post-commit-lifetime, branching, testing, whatever; all of the suggestions I've seen so far are IMO doomed to fail because they do not fix the underlying problem that not enough experienced manhours are being dedicated to Code Review for the amount of work (not the 'number of commits', the amount of *energy* to make changes to code) in the system. A pre-commit-review system doesn't reduce the amount of work needed to get a feature into deployment, it just changes the nature of the process. At the moment revisions sit unloved in trunk until they fossilise in; in that system with the current balance of time they would sit unloved in a bugzilla thread or branch until they bitrot into oblivion.
There *are* strategies that could be implemented (like the review-and-super-review processes used by Mozilla) that *can* streamline the process, but as has been said elsewhere, that *still* needs top-level direction. The members of Foundation staff who consistently get involved with these discussions do generally seem only to have hold of the deckchairs, not the wheelhouse.
--HM
On Wed, Jun 1, 2011 at 4:41 PM, Happy-melon happy-melon@live.com wrote:
+1. Pre-commit-review, post-commit-lifetime, branching, testing, whatever; all of the suggestions I've seen so far are IMO doomed to fail because they do not fix the underlying problem that not enough experienced manhours are being dedicated to Code Review for the amount of work (not the 'number of commits', the amount of *energy* to make changes to code) in the system. A pre-commit-review system doesn't reduce the amount of work needed to get a feature into deployment, it just changes the nature of the process.
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 Thu, Jun 2, 2011 at 2:28 AM, Chad innocentkiller@gmail.com wrote:
That's because our phpunit setup is incredibly fragile. I'm trying to fix it.
-Chad
Instead of the "auto" tests like we have now, could we some how (might need another testing suite?) perhaps have some sort of commit hook that tells the testing server a new commit has been entered to do a test against it up to that point compared to the "guessing" what commit between two points did it, although if there is multiple sets of tests going due to the closeness of commits, they may all fail but it would be easier to track back to the first one that failed.
On Thu, Jun 2, 2011 at 4:01 AM, Trevor Parscal tparscal@wikimedia.org wrote:
I believe the way forward involves using pre-commit review, requiring test coverage to pass review, and developers working in branches at all times. SVN may be a pita when it comes to branches, but that's a solvable problem.
- Trevor
Wouldn't working in branches just hide the mess and move it else where?, the same problems would still arise and could potentially increase workload (eg: if someone finds a bug and works on fixing it whilst someone else has noticed the same thing and worked on it as well but not checked to see if anyone else has).
On Wed, Jun 1, 2011 at 4:50 PM, Brion Vibber brion@pobox.com 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.
Right. And just to weigh in quickly on the resources issue -- the review/deploy/release train is clearly not moving at the pace we want. This does affect WMF staffers and contractors as well, but we know that it's especially frustrating for volunteers and third party committers. We kicked around the idea of a "20% rule" for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on "service" work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.).
An alternative model is to have rotating teams that do this work. I personally prefer the 20% model because it gives more consistency/predictability and less churn, but I'm curious what other folks think, and I'm comfortable with us continuing this discussion openly on this list.
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
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@lists.wikimedia.org [wikitech-l-bounces@lists.wikimedia.org] on behalf of Erik Moeller [erik@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Jun 1, 2011 at 9:09 PM, Russell N. Nelson - rnnelson rnnelson@clarkson.edu wrote:
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".
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
Chad wrote:
On Wed, Jun 1, 2011 at 9:09 PM, Russell N. Nelson - rnnelson rnnelson@clarkson.edu wrote:
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".
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.
+1
Thank you for posting this Erik.
I initially didn't reply on this thread because to me it just seems to be the same as the 3 or 4 times we've had the same discussion before. No technical or procedural change is going to solve a problem that is ultimately caused by a lack of time assigned to it. I was somewhat disappointed by the suggestion that the onus should be on volunteers to find a reviewer for their code, as this is just as unworkable without staff time assigned to doing it - and perhaps could be even more demotivating than the status quo.
Having review be a portion of staff time was always how I assumed it would be arranged. The justification for not assigning time to review has often been that one person or team doing it all the time would result in burn-out, and I agree with this. As a (part-time) developer I certainly wouldn't want to spend all my time doing service work, even for just one or two weeks at a time. The idea of having a single person/team assigned for each deploy/release cycle is nice, but I think that it would lead to burn-outs and conflicts with existing responsibilities those people have. Whatever model is implemented, I hope it can finally be successful.
I reject the notion that we need a change in our review processes. Empirical evidence shows that when staff time is assigned to it, our code review system *does* work. (Of course there are always minor changes that would improve efficiency, such as automating some of the reporting tasks that have traditionally been done manually.)
Updates on how this is going would be nice, as again it took MZMcBride to bring it up before anyone senior posted about it - and this is still the big issue for volunteer developers.
Cheers, Robert
On 2011-06-01, Erik Moeller wrote:
On Wed, Jun 1, 2011 at 4:50 PM, Brion Vibber brion@pobox.com 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.
Right. And just to weigh in quickly on the resources issue -- the review/deploy/release train is clearly not moving at the pace we want. This does affect WMF staffers and contractors as well, but we know that it's especially frustrating for volunteers and third party committers. We kicked around the idea of a "20% rule" for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on "service" work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.).
An alternative model is to have rotating teams that do this work. I personally prefer the 20% model because it gives more consistency/predictability and less churn, but I'm curious what other folks think, and I'm comfortable with us continuing this discussion openly on this list.
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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Thu, Jun 2, 2011 at 9:20 AM, Robert Leverington robert@rhl.me.uk wrote:
I reject the notion that we need a change in our review processes. Empirical evidence shows that when staff time is assigned to it, our code review system *does* work. (Of course there are always minor changes that would improve efficiency, such as automating some of the reporting tasks that have traditionally been done manually.)
I agree that it's worth trying to add staff time without materially changing the process, see how that works, then change things as needed.
Updates on how this is going would be nice, as again it took MZMcBride to bring it up before anyone senior posted about it - and this is still the big issue for volunteer developers.
FWIW, I was planning to bring this up at the staff meeting following the Berlin hackathon. I said to a number of people that I wanted to have a discussion about code review, both before and during the event. This didn't end up happening, partly because some felt that people that weren't around should be involved, or that the entire discussion should be had in public, and partly because we were doing all sorts of other things. I think MZ knew about my plans and the lack of a post-conference update saying "here's the outcome of our discussion, what do you think" may have driven him to bring this up. I care about this issue, and I agree with MZ on most of the basics, but I'm also a busy guy who took some time off after the conference to focus on school.
So having said the obligatory "but I care too!" thing, you are right that it seems, at least to people outside the office (both staff and non-staff: apparently there are lots of hallway discussions about CR these days, but they happen thousands of miles from my desk so working for WMF doesn't help me there), that there isn't a lot of attention given to this. Whether that's actually the case or just the impression that's being created due to lack of communication doesn't matter all that much; we can and should fix it.
Roan Kattouw (Catrope)
On Thu, Jun 2, 2011 at 2:58 AM, Erik Moeller erik@wikimedia.org wrote:
An alternative model is to have rotating teams that do this work. I personally prefer the 20% model because it gives more consistency/predictability and less churn, but I'm curious what other folks think, and I'm comfortable with us continuing this discussion openly on this list.
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)
On Thu, Jun 2, 2011 at 2:26 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
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.
To keep the ball rolling, I've started a draft here, based in part on your comments: http://www.mediawiki.org/wiki/Development_process_improvement/20%25_policy
Please be bold and/or add your thoughts. As per the note on the page and other comments in this thread, a policy like this isn't a replacement for dedicated staff supporting the code review and release process; it's merely one component of ensuring appropriate capacity is available for relevant work.
Erik Moeller wrote:
On Thu, Jun 2, 2011 at 2:26 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
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.
To keep the ball rolling, I've started a draft here, based in part on your comments: http://www.mediawiki.org/wiki/Development_process_improvement/20%25_policy
Please be bold and/or add your thoughts. As per the note on the page and other comments in this thread, a policy like this isn't a replacement for dedicated staff supporting the code review and release process; it's merely one component of ensuring appropriate capacity is available for relevant work.
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
On Mon, Jun 13, 2011 at 11:39 PM, Erik Moeller erik@wikimedia.org wrote:
To keep the ball rolling, I've started a draft here, based in part on your comments: http://www.mediawiki.org/wiki/Development_process_improvement/20%25_policy
As a quick update, yesterday we had a conversation to think about how we can make this real. In this discussion, the participants were the Engineering Program Managers, Sumana and me. If you want the play-by-play, you can see Sumana's notes here:
http://etherpad.wikimedia.org/Dev-Process-Improvement
The high level summary is that within the management group, there's consensus that this is a good idea, and that we need to make it real ASAP, principally within the parameters of the aforementioned policy page.
The practical barriers are that we 1) need to ensure that we're not adding to people's existing massive workload without taking something off, in order to do this, 2) we need to plan for skills development related to code review and deployment.
With regard to 1), managers are beginning conversations with their reports with regard to project allocation, and we're working towards getting every full-time engineer freed up ASAP to make that 20% commitment.
With regard to 2), RobLa and Sumana are starting to think about a code review/deployment boot camp (more WMF-focused than a hackathon, but ideally with some options for remote participation) -- again, see the notes for what's been discussed so far; Rob's going to talk more about this in a couple of weeks or so (he's off next week).
All best, Erik
On Wed, Jun 1, 2011 at 5:58 PM, Erik Moeller erik@wikimedia.org wrote:
Right. And just to weigh in quickly on the resources issue -- the review/deploy/release train is clearly not moving at the pace we want. This does affect WMF staffers and contractors as well, but we know that it's especially frustrating for volunteers and third party committers. We kicked around the idea of a "20% rule" for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on "service" work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.).
Expect to hear more from the other EPMs on this subject, but this is a no-brainer for General Engineering to get on board with (we're already pretty much doing this).
Here's an immediate challenge we face: in order to release 1.18, we need to get the red line in this graph to zero: http://toolserver.org/~robla/crstats/crstats.118all.html
As of midnight UTC on June 3, we were at 1594 revisions. Doing the math, in order to get close to zero by July 15, we have to review 265 revisions/week. That means:
2011-Jun-03 1594 2011-Jun-10 1329 2011-Jun-17 1064 2011-Jun-24 799 2011-Jul-01 534 2011-Jul-08 269 2011-Jul-15 4
So, can we make it a goal to get to 1329 by this time next week? That first 265 should be a lot easier than the last 265, so if we can't get through the first 265, then we don't really stand a chance.
Rob
On Fri, Jun 3, 2011 at 3:33 PM, Rob Lanphier robla@wikimedia.org wrote:
So, can we make it a goal to get to 1329 by this time next week? That first 265 should be a lot easier than the last 265, so if we can't get through the first 265, then we don't really stand a chance.
I accept this challenge! My 20% is committed for next week... are y'all with us? :D
-- brion
On Fri, Jun 3, 2011 at 7:45 PM, Brion Vibber brion@pobox.com wrote:
On Fri, Jun 3, 2011 at 3:33 PM, Rob Lanphier robla@wikimedia.org wrote:
So, can we make it a goal to get to 1329 by this time next week? That first 265 should be a lot easier than the last 265, so if we can't get through the first 265, then we don't really stand a chance.
I accept this challenge! My 20% is committed for next week... are y'all with us? :D
Which day? I'll pencil it in as well :)
-Chad
On 04/06/11 00:33, Rob Lanphier wrote:
Here's an immediate challenge we face: in order to release 1.18, we need to get the red line in this graph to zero: http://toolserver.org/~robla/crstats/crstats.118all.html
It is a thin red line. Lets just pretend we are in the Crimean Peninsula.
Rob Lanphier wrote:
Here's an immediate challenge we face: in order to release 1.18, we need to get the red line in this graph to zero: http://toolserver.org/~robla/crstats/crstats.118all.html
As of midnight UTC on June 3, we were at 1594 revisions. Doing the math, in order to get close to zero by July 15, we have to review 265 revisions/week. That means:
2011-Jun-03 1594 2011-Jun-10 1329 2011-Jun-17 1064 2011-Jun-24 799 2011-Jul-01 534 2011-Jul-08 269 2011-Jul-15 4
So, can we make it a goal to get to 1329 by this time next week? That first 265 should be a lot easier than the last 265, so if we can't get through the first 265, then we don't really stand a chance.
Thanks for posting this. This seems pretty reasonable. If there's any support or help that non-developers can offer in this process, please update the mailing list with information as to how. A lot of people are keen on seeing some steady progress forward. :-)
MZMcBride
On Sun, Jun 5, 2011 at 9:13 PM, MZMcBride z@mzmcbride.com wrote:
If there's any support or help that non-developers can offer in this process, please update the mailing list with information as to how. A lot of people are keen on seeing some steady progress forward. :-)
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
On Wed, Jun 1, 2011 at 8:23 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed.
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 31.05.2011, 22:41 Rob wrote:
So, there's a number of possible solutions to this problem. These are independent suggestions, but any of these might help:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code.
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.
The obvious problem with the 72 hour time limit is that it imposes unnecessary time limits, adding lots of to the complexity, chaos and stress - possibly a net-negative, but it's hard to say for sure.
That said, at least it's an actionable idea.
It's pretty clear to me that the inconvenience that's been avoided in this area by shooting down ideas rather than just taking action and working out the details as we go is equivalent to a meer fraction of the effort that's been put forth in the form of demanding a perfect solution and generally resisting change.
We should probably also not be setting the stakes so high, it's making the cost of failure unnecessarily expensive. One project should change their review and release strategy, and see if that can be worked out and fine tuned. Then we can apply those lessons to the code base in general. This is especially useful if a strategy is radically different than what we have now.
- Trevor
On Tue, May 31, 2011 at 12:37 PM, Max Semenik maxsem.wiki@gmail.com wrote:
On 31.05.2011, 22:41 Rob wrote:
So, there's a number of possible solutions to this problem. These are independent suggestions, but any of these might help:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code.
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]])
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Tue, May 31, 2011 at 12:37 PM, Max Semenik maxsem.wiki@gmail.com wrote:
On 31.05.2011, 22:41 Rob wrote:
So, there's a number of possible solutions to this problem. These are independent suggestions, but any of these might help:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code.
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?
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.
"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.
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.
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.
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
On 31/05/11 23:13, Brion Vibber wrote:
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.
What prevents us to actually implements this? We could just restrict the /trunk/ to a few merging people, they will only take care of merging revisions set from other branches.
Developers then build their fixes / features in their own branches and get it reviewed / signed-off by others. Once done, it is submitted to the merge team.
This is much like the REL branches which only a few people are able to merge trunk changes in. Just with another layer :-)
The VCS would be up to the developer (thus solving the "migrate from svn to git" issue in the meantime). Subversion will only be used to actually merge patch sets in trunk or REL branches.
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.
Then we need merge windows. A date by which everyone would have to make sure their patch is ready: tested heavily, reviewed by people actually knowing the part of code impacted, test written, style fixed, i18n ...
If the patch does not make it in the current merge windows, it will apply for the next one.
On Wed, Jun 1, 2011 at 1:13 AM, Brion Vibber brion@pobox.com wrote:
"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.
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 would wholeheartedly supoport the idea of making tests a mandatory accompaniment to code. One latest example: we made latest security releases without tests. This is bad not only because bugs not tested for *will* eventually reappear, but also because in my experience the process of writing tests helps to discover more problems with code. If compared with manual tests, writing low-level unit tests allows to look at possible problems more methodicallly, uncovering more problems.
Ideally, I would like to see the same policy as Chome has: 1) All reasonably testable code must be accompanied with automatic tests, period. (It's easier to enforce for them because they practice pre-commit reviews.) 2) To commit, hang around on IRC. After committing, wait for buildbot to post test results of your commit. If something's broken, fix it ASAP, otherwise you'll be reverted.
To make this reasonable, we would also need to run tests for branches on the central server, too - CruiseControl doesn't look flexible enough for this.
Max Semenik wrote:
I would wholeheartedly supoport the idea of making tests a mandatory accompaniment to code. One latest example: we made latest security releases without tests.
Actually, we have made security releases that *broke* tests. (ie. tests got outdated)
This is bad not only because bugs not tested for *will* eventually reappear, but also because in my experience the process of writing tests helps to discover more problems with code. If compared with manual tests, writing low-level unit tests allows to look at possible problems more methodicallly, uncovering more problems.
(...)
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.
Rob Lanphier wrote:
On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar neilk@wikimedia.org wrote:
Are we all in deadlock or something?
<snip>
independent suggestions, but any of these might help:
- We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code.
Russell N. Nelson - rnnelson wrote:
Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review.
And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says.
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
wikitech-l@lists.wikimedia.org