I've had some fun clicking the "Play patch bingo!" link repeatedly and think it is a very fine addition to the Contributor links block. One thing that really impressed me was the huge number of patches that seem like a good idea, whose authors assert that the patch is ready to commit, and that have several "+1 I reviewed and liked" comments. All of this made me think a little more about the undead issue of whether we need more core committers and how patches can get reviewed and processed in a more organized manner. Then I had an idea. Here it is.

In order to 1) motivate the masses of developers to review patches, 2) guarantee that patches don't get buried in the queue and never get read, 3) have a way to *measure* whether Dries and Steven can keep up on their own, why don't we implement a patch review schedule? I imagine it working something like this:

  1. New patches are put into a first-in-first-out queue. This queue is the order in which committers will review the patch.
  2. Committers must (must is a very flexible word here... more about expectations than rules) review the top patch before reviewing the one right in line behind it, kind of like a judge has to review court cases in a fixed order.
  3. The "up next" patches get well advertised so that developers and those interested in a particular patch know when to do their lobbying and when to make sure that things get reviewed by a critical mass of developers so that the committer has enough support to make a decision.
  4. Every patch review must result in a status change or update comment. This is the history or review trail of a patch. This occurs now anyway, so this is no big change but rather an acknowledgment of the expectation that the committers never "silently" look at a patch and then move it to the back of the queue.
  5. Every patch review results in either a committ, having the patch removed from the queue (won't fix), or being sent to the back of the queue with concrete criteria and expectations for when it gets reviewed next. (I'm not suggesting that if Dries writes "remove this space and send a new patch" and someone does this 5 minutes later that the patch has to wait two weeks until it gets to the top of the list)

That's the extent of what I propose to add/change. Here's what I hope that we would gain from it. I expect that the testing efforts of the developer community would become more focused, meaning we would be more likely to be testing the same patch at the same time. Why? Let's say that I have a patch in the queue and it is in place 5. That means very soon, Dries or Steven will be reviewing it. My big chance! Since I know that the opportunity is at hand and that the price of not getting it committed this time around will be high (put back to the end of the queue), I will make sure the patch still applies and then get on IRC and say "hey chx, walkah, killes and amazon, please test this patch today... it is up for review and I need some critical mass". The result will be that more eyes and intentions will be focused on the patch at that moment, which hopefully will give Dries the info he needs and has asked for when it comes to review time.

Please note that I am not suggesting that committers should be forbidden from following a patch that they're interested in and committing patches from the middle of the queue. I'm proposing a system for addressing those days when they wake up in the morning and say "well, let's see.... maybe I'll review patches today. Let's take a look at that patch queue." The value of the list is that the whole world will know that very soon, the patch on the top of it will be reviewed and when it is, that is the biggest chance for a while that it will get committed. Furthermore, nobody submitting an issue will have the voice in their head saying "hope Dries is online today so he sees this patch before it gets burried in the queue". Instead you'll know that your patch *is* burried at the end of the queue, but that it will definitely, 100% guaranteed, get reviewed eventually.

We can also train the masses to participate more in the review process. If the "up to bat" and "on deck" and "in the hole" issues are visible on drupal.org, and if we provide nice instructions for reviewing patches in such a way that people get the idea "oh, this is something I'm expected to do", I think we'll have a higher review rate.

Another thing that such a queue would open the door for is an issue freeze that comes before a code freeze in the development cycle (of course critical bugs will always be handled ASAP and not be put in any queue). Maybe two weeks before a code freeze we can decide that all new issues will go into the queue for the next cycle, and the timing of the code freeze will depend on working through the current queue. As it is, there are dozens of small bugfixes in the queue which will likely not see the light of day before the code freeze or release. This method of organizing the queue might help give us more concrete goals for each stage in the cycle.

Finally, and this is the part that is hardest to say since I love Dries and Steven and think they do a bang-up job, a system like this would make it very clear if the system isn't keeping up with demand. If we can't even make it through the whole queue once per development cycle, there is a bottleneck that is restraining our progress. Perhaps we'd decide, for example, to not increase the number of committers, but rather that we need a pre-commit team (5-10 people) who are the ones who review every patch, and only the ones they approve get sent to Dries and Steven. A solution like that would make two similar queues; the one I just described with everything in it which will get reviewed in order by memebers of the pre-committ team, and a queue for Dries and Steven that functions the same way but only contains patches that have passed the pre-commit review phase and was promoted by a member of the pre-commit team (perhaps members of the pre-commit team cannot promote their own patches).

Ok - there it is. Flame me!

cheers,

Robert

Comments

robertDouglass’s picture

Another benefit that I forgot to mention is that the price of ignoring crappy issues is higher. Take the case where a really important and really anticipated patch is in place 12 and the 11 issues before it are crap, meaning submitted by bad coders, not addressing important issues and generally unlikely to make core. Anyone interested in the good issue in place 12 will be saying "hey cats, can't we clean the litter box here, I mean, what is all this crap keeping us from getting to the real deal". And *whooosh*, 11 crappy issues are marked to "get a life" or at least "won't fix" and removed from the queue. The price of ignoring issues is high already; our good issues get lost in the crap. The real price we pay for this is delayed and comes in the form of a bloated queue that a core committer won't touch with a ten foot clue-bat. By requiring that we clear the crap to get to the meat the price is moved upfront, which might seem more expensive, but the slim and trim patch queue that will result will be worth it and the overall price of crappy issues will be lower.

robertDouglass’s picture

Category: feature » bug
Status: Active » Needs review

[dirty trick] I'd like to have better ways of making issues like this show up on the devel-list.

robertDouglass’s picture

Category: bug » feature
Status: Needs review » Active

[/dirty trick]

robertDouglass’s picture

The more I think about having a patch review team the more I like it. The team, as stated in the first post above, would be responsible for reviewing and testing patches from the general queue. Ony after a member of the team promotes a patch would it appear in the queue for the core committers. This would make the core committers' work much easier and would save them the time of digging through all the crappy patches.

Nedjo Rogers made an excellent point in the developers' mailing list:

When we call for more core committers, we're not saying we need more people who can press a button when everything is all ready. We're saying, rather - or I am, anyway, as I have repeatedly over the past couple of years, but to no avail - that we need to bring more people into formal, recognized decision-making roles.

Where are all these highly skilled, dedicated reviewers supposed to come from? The motivation to commit time and energy doesn't come from nowhere.
It certainly doesn't come from the experience of investing gobs of time and energy only to have it sit there forever in limbo. That's what I hear Gerhard saying--and he couldn't be more correct.

This sort of dedication comes, rather, from a feeling of recognition, membership, involvement, and a direct role in decision making.

Having a patch review team would do a lot to address Nedjo's concerns.

killes@www.drop.org’s picture

Robert, I don't want to offend you, but I think that in all the time you've spent thinking and writing about this, you could have played patch bingo for about 15 patches...

Earlier attempts (mainly by Nedjo) to introduce more formalized procedures did at best receive a luke warm reaction if any at all.

In fact, such a team already exists. Everybody who wants to can review patches. And how much a review counts is probably determined by some internal repesct value in the brains of our core committers.

robertDouglass’s picture

I was ready to let the issue drop until I really inspected the patch queue and realized how much work is being wasted and lost.

Bèr Kessels’s picture

@Robert: Goot thinking, but what a long read.

To me it summarises like this: "Order patches chonologically, and let *everyone* walk trough them like that".
That makes a *lot* of sense to me. I see the same problems. And I spent a lot of time reviewing patches. But moer often then not these patches are lost in eternety, as is my testing and reviewing effort.

@killes: True. but these revewing efforts make a big chance of getting lost in oblivian. really. A **lot** of patches just got lost.

@ everyone: We need a few things:
* A way to recognise good from bad.
* A more efficient way to say: "not going to happen". These patches now never disappear, but hang around for eternety. I often take on teh features from the back, just closing fifty on a day. yet still the queue grows very fast.
* A way to get patches to not conflict. There is nothing worse then having spent 10 hours over the whole tread to findetune your patch. Only to find out it is totally foobar *again* because patch X jsut went in yesterday.

Overall/ I like the idea of walking trough the queue in a pre-defined order. +1 for that.

Richard Archer’s picture

I don't like patch bingo at all. I think it only serves to scatter the concentration of review effort far and wide across the whole spectrum of patches. When instead, concentrating that effort on 20 patches would have them refined, reviewed and committed (or rejected) in a week or so.

When I have time to spend on Drupal I would much rather be presented with a list of 10 or 20 patches or issues that are likely to be committed once they have been fully developed. That way I know that the time I'm spending is not a complete waste.

As for picking nits in Robert's proposal...

I'm not sure I like the way this system encourages political manouvering to get a patch reviewed. I would prefer a patch to make it to the big time based on its own merit rather than the time its author has to spend time currying favour on IRC.

Something bothers me about the FIFO queue too, but I can't quite put my finger on it. Something perhaps to do with the time it takes to review and decide on the fate of a patch. Sometimes this could take days or even weeks and while this is going on that issue is just sitting at the top of the queue with not much happening to the ones behind it.

There needs to be some way to keep the head of the queue fresh. Maybe have the FIFO queue for the backlog of issues, but the head of a queue is a heap of 10 or 20 issues that are all being reviewed/refined/judged in parallel. As one issue falls off the heap a new issue pops in from the queue.

I think this is an important issue, but just like the patch that deals with the troublesome BASE tag in Drupal pages, I fear that even though it is worthy and maybe even critical, it's just going to linger in the queue unloved and unheeded for a year or more until one day Bèr closes it along with 49 other fine ideas.

Chris Johnson’s picture

Title: Patch review queue » Here's a suggestion for a tool or method

I am not suggesting the following because I support it, but rather am presenting this idea to prompt ideas from other people.

What if one could not submit a patch until one had enough review points? For example, let's say every CVS account holder was given 3 points to start and perhaps some core members were given 4 or 5.

Every time a patch was submitted, it cost 1 point to do so (or perhaps, any number of patches as long as they were all to one issue).

In order to get more points, one would need to review/test other patches and issues, receiving one point for each substantive review or test.

I don't think such a system would be easy to implement, nor that many would be in favor of it.

However, it is a way to address the problem of not enough testing/reviewing of patches so as to clear the queue and get good work committed. I am as guilty as anyone of submitting patches for my ideas and not testing other people's patches as often as I should.

Also, I think this is only one aspect of the problem set that Robert is addressing. As Richard points out, there needs to be a way to get the top 10 or 20 most desirable patches reviewed and committed, so that good work does not get lost amongst the larger number of patches many of which are not desirable or ready.

I look forward to discussing ideas on how to improve our throughput in Amsterdam.

chx’s picture

Title: Here's a suggestion for a tool or method » Patch review queue

Chris, this idea is lovely. It will IMO need a more query-based review, with required checkboxes like 'does it apply' 'is it needed' etc etc

Richard Archer’s picture

Now, even without a points-based system, a query-based review with checkboxes would really, really make the review process easier.

It would also make it easier to see the results of reviews... instead of having to extract the info from plain-text reviews the patch owner (and Dries) could just look at a summary:

2 reviewers said "did not apply"
1 reviewer said "not needed"
1 reviewer said "not up to Drupal quality"
5 reviewers said "ready to apply"

I give that idea a big +1 and I wish I had time to start coding it!

robertDouglass’s picture

Some refinements and modifications to the original ideas based on recent discussions

  • The idea of two queues is really taking hold in my mind. The first queue is a FIFO where a team of appointed reviewers (around the size of the security team) has the power to either:
    1. promote the patch to the second queue
    2. close the patch with a "won't fix" or "doesn't apply"
    3. send the patch to the back of the list with a set of requests (code style, patch doesn't apply etc)
  • The second queue comprises of patches that are genuine candidates for inclusion in core, and thus need review by core maintainers. Ideally there would be a way for Dries or whomever to set a flag "Currently reviewing" so that developer attention from the entire community could be focused on the handful of patches that have the greatest chance of making it through the gate.
  • The "follow up" page on issues gets more fields to help structure reviews.

Salient in the above is that only a privileged user role can promote a patch from the first queue into the second. As a further constraint, I would like people to think about the advantages or disadvantages of enforcing the FIFO aspect of the first queue by programmatically allowing only the current top 1-3 patches to be promoted to the second queue. I personally think that unless we build in a motivation for reviewing the patches in the first queue, it will be business as usual.

We all know that reviewing is essential to the health of Drupal, but the lack of structure makes reviewing random patches little more than a semi-productive way to procrastinate. If we make reviewing the patches at the top of the queue a prerequisite for getting to patches at the bottom of the queue, people will be motivated to keep the queue flowing.

One more important comment. The queue under these guidelines will be much smaller than the monster we've created today. If you are reading my proposal and thinking "gosh, I just wouldn't be able to stand to wait for 500 patches to get reviewed before mine gets it's chance", you needn't worry. I predict that the average time a patch will spend in the first queue will be less than one week, and if the average time rises above one week, red warning lights need to start flashing on the site.

bdragon’s picture

Version: x.y.z » 7.x-dev

Bumping for irony.

Resetting version.

The Patch Spotlight helps some regarding this, but much more can be done....

greggles’s picture

Status: Active » Closed (won't fix)

3 years after the last mention I think this is won't fix.

Simpletest has taken some of the roles described here. Feel free to re-open if you still like the idea, but I think the core issue queue is an unlikely place to brainstorm/implement this. I think a core-dev-summit presentation might be better.