Running a CommitFest

From PostgreSQL wiki

Revision as of 01:41, 27 December 2009 by Itagaki (Talk | contribs)

(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

Initial Patch Assignment

  1. Several days to a week before the start of the CommitFest, post an email to pgsql-hackers and pgsql-rrreviewers and ask for volunteers. It's useful to include links to Reviewing a Patch and RRReviewers. Sample e-mail
  2. A few days later, post a reminder that you still need more volunteers. Also, remind patch authors to add their patches to commitfest.postgresql.org by the deadline. For CommitFest 2009-07 and 2009-09, the deadline was 2009-MM-15 00:00:00 GMT.
  3. Subscribe all reviewers to pgsql-rrreviewers. Most of them already will be. If this is your first time through, you may need to get yourself added as a co-moderator (or ask an existing moderator to do it).
  4. Get reviewing assignments out PROMPTLY, within a few hours of when the CommitFest starts. Even though there may be last minute submissions, you can usually have a tentative list of who will get what patch worked out in advance. Don't assign reviewers to patches that committers have already claimed unless they so request. Assign the hardest patches to the most skilled reviewers, in areas they know something about if you have an idea where that person's skill-set is. Leave the smallest and least important patches for later review rounds (this is somewhat subjective, of course). Assign patches that are likely to be controversial to people with good writing skills, in the hopes of getting a good discussion going.
  5. When you send out the initial review assignments, ask for reviews to be completed within 5 days. Tell reviewers to request a new assignment when they finish their current one. Remind reviewers of the importance of updating commitfest.postgresql.org whenever they post a review. They should BOTH add a "review"-type comment AND update the patch status to Waiting on Author, Ready for Committer, Returned with Feedback, or Rejected.

Patch State Terminology

In progress states:

  • Needs review: Waiting for a reviewer to do an initial or re-review
  • Waiting on Author: we're expecting them to resubmit during this CommitFest
  • Discussing review results: review is complete but next step for this patch not clear, discussing on the pgsql-hackers list
  • Ready for committer: Review complete, no issues found by the reviewer left to correct, ready for a final review by a committer. There is a "Committer" field in the patch detail that lets committers note they have or intend to commit a particular patch.

Final patch states:

  • Returned With Feedback: we'll review a new version for the NEXT CommitFest if one has been submitted
  • Rejected: we don't want it in this form, ever
  • Committed: applied by a committer, no remaining issues

Generally, once a patch reaches one of the final states, it's no longer considered part of the active CommitFest anymore.

Followup, Followup, Followup

Once initial review assignments are out, the hard part begins: getting all of the patches committed, returned with feedback, or rejected. This is very, very difficult unless commitfest.postgresql.org is up to date. It often isn't, so expect to spend a lot of time adding new patch versions (that the patch authors failed to add) and new reviews (that the reviewers failed to add) to the system, and updating statuses to whatever is appropriate.

Every day or two, scan through the list of patches and do the following, depending on what state each patch appears to be in:

  • "Waiting for Author" for more than 5 days: send a note indicating that you are marking it "Returned with Feedback", then do so.
  • "Needs Review" for a similar length of time: remind the reviewer about the patch. This often happens when the author posts an updated patch: the reviewer reviews, but then has to be prodded to check over the updated version ("Fred, is this Ready for Committer?").
  • Bogged down in discussion: emphasize the importance of reaching a conclusion and getting the patch committed or punted.

Once a patch reaches "Ready for Committer", you don't need to worry about it any more until a committer looks at it. But if too many build up in that state, it may be worth prodding pgsql-hackers in an attempt to drum up some more committer activity. You do need to make sure that patches that have been looked at and criticized by a committer get marked as "Waiting for Author", "Returned With Feedback", or "Rejected" as appropriate. In some cases the committer will update the status themselves, but you shouldn't count on that.

As reviewers finish their existing patches, you need to assign them new ones, at least until you run out of things that need to be reviewed. This is pretty easy: reviewers will often pick something themselves. Even if they don't, there often aren't many to choose from after the first round of reviewing is completed.

Activity Transitions

All new patches start in the "Needs review state".

State Action By Activity Next state
Needs review Reviewer Review discovers problems with clear correction work Waiting on author
Needs review Reviewer Reviewer not certain how to resolve patch issues Discussing review
Needs review Reviewer No issues discovered with patch Ready for committer
Needs review CF Manager 5 days passed with no activity: reminder/reassign Needs review
Discussing review Author Resolution proposed Waiting on author
Discussing review Reviewer Issues clarified, returning to review Needs review
Discussing review CF Manager Stall or 5 days passed with no clear next step, patch desirable Returned with feedback
Discussing review CF Manager Stall or 5 days passed with no clear next step, patch seems undesirable Rejected
Waiting on author Author Updated version posted Needs review
Waiting on author CF Manager 5 days passed with no activity Returned with feedback
Ready for committer Committer Unhappy with any variation of this approach Rejected
Ready for committer Committer Committed Committed
Ready for committer Committer Finds additional issues Waiting for author

Notes about some less common situations here:

  • Once a patch has reached "Ready for committer", new versions are expected to be re-reviewed by the committer rather than the initial reviewer--although the reviewer can always join in.
  • Patches sometimes jump to "Returned with feedback" or "Rejected" more quickly than suggested here. Usually this is when the reviewer, CF Manager, or committer believes the work to resubmit is not possible to accomplish in the remaining time during this CF, or if there's some really fundamental issue with the patch but the feature is obviously desirable. Authors will sometimes surprise us with the speed of their updates however. Unusual transitions can always happen via the judgement of the CF manager or a committer, such as returning a "returned with feedback" patch to active work. The main goal of this chart is to provide better guidelines for reviewers and authors about how to act and how fast they're expected to do so, not to describe every possible path through the process.
  • From the date of first review, the goal is to have any patch into a final review state (or at least ready for a committer) within 15 days of that review being posted. Patches still being worked on at that point can, at the discretion of the CF manager or a committer, be marked "returned with feedback" for this CF, in the interest of fairness to other patch submitters.
  • Time running out on the CF itself will accelerate the expected response time window here. If there's only a week left in the CommitFest, and a patch has gone to "waiting on author", the author may not be given the usual 5 days to wrap that up before having their patch moved to a final state.

Closing the CommitFest

Remember that your goal here is to make the CommitFest END. Patches need to reach a conclusion. It's important that every patch gets a fair shake, but many of them will not be committable, or will be committable only with work that the authors aren't willing to do in a short period of time. Getting these out of the way as quickly as possible helps everyone focus in on the patches that ARE potentially committable.

If you are handling 60 patches in a 30-day CommitFest, about 2 patches per day need to get committed or returned/rejected. The first week should go a bit faster. If you are 15 days into a 30-day CommitFest with 60 patches, and you have 34 patches left, you are in bad shape. If you have 26 patches left, you are in pretty good shape. Of course, you can't always control the rate at which patches get done. In particular, you can't control commits at all. But it gives you a rough guide to how much you need to worry and/or apply pressure to whoever is holding things up. Leaving all the decisions to the end is a bad idea. You want to cut off the stuff that isn't going to make it as early as possible.

During the five CommitFests from 2008-09 to 2009-11, the average pace of committed patches was approximately one per day. Since about 42% of the submitted patches during the last of those three were either returned with feedback or rejected, the actual progress rate through the queue has been closer to 52/month.

Personal tools