Thread: Documenting commitfest Rules

Hi Tomas, Everyone.


This seems like a good thing to discuss separately from the original
thread.

On 2018-03-02 22:06:32 +0100, Tomas Vondra wrote:
> Can you guys please point me to the CF rules that say this? Because my
> understanding (and not just mine, AFAICS) was obviously different.
> Clearly there's a disconnect somewhere.

I think we generally have documented the CF process very poorly.  Way
too much of it was discussed in-person at developer meetings (with notes
taken), is buried in deep threads arguing about release management or
individual commitfests.  A lot of the wiki information is outdated too.


Thus I'm trying to summarize my understanding in this email. It probably
doesn't 100% match other people's understanding. After we've resolved
those differences, we should update the wiki page, and delete outdated
content.


A single Commitfest is commonly called CF.

== Goal ==

Commitfests primarily exists so:

- we do not accidentally loose track of patch submissions that deserve
  attention, that previously happened frequently
- that reviewers and committers quickly can find patches deserving their
  attention (i.e. ones in in needs-review and ready-for-committer
  respectively)
- that there's a single point documenting the state of the patch, to
  avoid situations where different people interpret a thread differently
  without noticing.


== Entries ==

A commitfest entry is a patch, or set of patches, implementing some goal
(e.g. a new feature, some cleanup, improved docs). Each commitfest entry
is associated with one or more email threads, that discuss the feature.

A patch can be authored by multiple people, and the set of people
actively working on a patch can change.

Submitting a patch as a commitfest entry to a specific commitfest
implies a statement by the author that the patch needs input from
others. That input can be agreement on design decisions, high level code
review, testing, etc.

A CF entry can be in multiple states:

- Needs Review: this means that the patch author needs input to
  continue. While there may be some concurrent activity, the author will
  stall at some point without he input.

- Waiting for Author: there has been feedback to the author that makes
  continuing review unproductive. The most desirable case for that is
  because there has been a detailed review describing issues with the
  current implementation, which the patch author is expected to address
  or discuss.  Another reason might be that the current patch doesn't
  apply to the source tree anymore, and a rebased version of the patch
  is needed.

- Ready for Committer: reviewers have reviewed the patch and are,
  presumably after a few rounds of patch updates, happy with the current
  state and think the patch needs the attention of a committer. That can
  be because the patch is ready to commit, or because the design has
  evolved as far as it can without validation from somebody potentially
  willing to commit the patch.

- Committed: The patches associated with the CF entry have been
  committed.

- Rejected: The project has decided that the patch in the current form
  is not desirable.

- Returned with Feedback: The patch has gotten review indicating that it
  needs nontrivial changes, and the author did not have time and
  resources to perform them within the time of the current commitfest,
  or shortly thereafter.  It might be set because the author indicated
  so explicitly, stopped responding, etc.

- Moved to next CF: The patch was in needs review state, but either the
  current commitfest is about to end, or the patch is clearly in a state
  that is inappropriate for the current commitfest. The latter usually
  only matters in the last commitfest in a release cycle, more about
  that below.


== Commitfest Process ==

Commitfests start at a certain date. To avoid timezone issues, the
cutoff is when the start day has begun everywhere on the world.

All patches to be considered in that CF have to be submitted the day
before the start date, and either be in Needs Review or Ready for
Committer state.  That requirement exists because the goal of commitfest
is to provide input to patch authors so the patch can evolve and to not
forget patches that ought to be committed.

During a commitfest reviewers look at patches and provide feedback to
the authors. In the best case reviewers respond within a few days,
adapting the patch, answering changes, etc.

If the reviewers think a CF entry is ready for commit (or need design
input from a committer), the patch is set to Ready for Committer. Some
committer will hopefully come along and either commit the patch, or
explain what should happen in their opinion. Thus the followup states
can be Committed, Needs Review, Waiting on Author.

If patches are in Waiting for Author state for a while, after a
"warning" by the CF manager or reviewers, the patch will be marked as
Returned with Feedback, signaling that the project is not expected to
continue working on the patch.  If a patch needs extensive changes
(i.e. it's unrealistic they are made before the end of the current CF)
it often also can make sense to mark such patches as Returned with
Feedback, but input by the authors should be sought.

If patches are in Needs Review state at the end of the commitfest, the
patch will be moved to the next commitfest. If they are Waiting on
Author, the patch should be marked as Returned with Feedback.


== Last Commitfest ==

The last commitfest is the CF before the expected feature freeze date
for the current yearly postgres release.

A few extra restrictions are in place, to make it more likely to finish
features that have a realistic chance of ending up in the current
release.

No nontrivial patches that have not previously been submitted to an
earlier CF should be entered into the CF. Otherwise new and shiny
patches will take up all the bandwidth from patches that have been
worked on for a long time.

Additionally patches that cannot realistically be finished within the
last commitfest will aggressively be moved to the next commitfest early,
to avoid using up resources that should be used to finish features for
the current release.


== Commitfest Manager ==

The commitfest's manager is determined at the beginning of the
commitfest. Sometimes multiple people will serve that role for a single
CF, to share the load.

Their role is to attempt to ensure that patches in the commitfest are in
an appropriate state, to bring up when that's not the case, and to
attempt to get all patches a fair amount of attention.



What do others think, is this a reasonable description of the *current*
process? Where do you think it is inaccurate?  I'd appreciate if we
could discuss *improvements* to the process separately. Let's first
agree on where we are.


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Thus I'm trying to summarize my understanding in this email. It probably
> doesn't 100% match other people's understanding. After we've resolved
> those differences, we should update the wiki page, and delete outdated
> content.

> Commitfests primarily exists so:

> - we do not accidentally loose track of patch submissions that deserve
>   attention, that previously happened frequently
> - that reviewers and committers quickly can find patches deserving their
>   attention (i.e. ones in in needs-review and ready-for-committer
>   respectively)
> - that there's a single point documenting the state of the patch, to
>   avoid situations where different people interpret a thread differently
>   without noticing.

I think that third point is at best an idealized statement, and it's not
very reflective of actual practice.  We're not great about updating the CF
entry's state, and even if we were, I don't think there's a bright line
between Needs Review and Waiting On Author.  There may have been enough
feedback provided so that the author has something to do, yet not so much
that there's no point in further review.  So maybe there's something that
can be improved about the CF tool there.  But anyway you said you wanted
to summarize current actual practice, and this isn't quite what really
happens IME.

There are a couple of meta-goals as well, although I'm not sure whether
they belong in this document:

* Encourage people to review other people's patches.  This isn't just
to make the patches better, it's to make the reviewers better: they
gain familiarity with the PG code base.

* Ensure that committers don't have to *always* feel guilty about
not working on other people's patches instead of their own.  Otherwise
we'd just stay in CF mode all the time.

> Submitting a patch as a commitfest entry to a specific commitfest
> implies a statement by the author that the patch needs input from
> others. That input can be agreement on design decisions, high level code
> review, testing, etc.

... or even just that the author would like somebody to commit it.

Also, there's at least one rule you forgot to cover, concerning
asking people to review patches more or less proportionally to the
amount of patches they've submitted.  This is surely a lot squishier
than the other rules, but without it, nothing much happens.

            regards, tom lane

On 2018-03-02 18:08:00 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > - that there's a single point documenting the state of the patch, to
> >   avoid situations where different people interpret a thread differently
> >   without noticing.
> 
> I think that third point is at best an idealized statement, and it's not
> very reflective of actual practice.  We're not great about updating the CF
> entry's state, and even if we were, I don't think there's a bright line
> between Needs Review and Waiting On Author.  There may have been enough
> feedback provided so that the author has something to do, yet not so much
> that there's no point in further review.

Yea, I think it's a bit of hit/miss. But I do think that *if it happens*
the discussion around that often provides some clarification.


> But anyway you said you wanted to summarize current actual practice,
> and this isn't quite what really happens IME.

I think it's OK to state some of the aspirational goals, even if they
only halfway work.  And I think we should evolve the practices after
agreeing on what they currently are ;)


> There are a couple of meta-goals as well, although I'm not sure whether
> they belong in this document:
> 
> * Encourage people to review other people's patches.  This isn't just
> to make the patches better, it's to make the reviewers better: they
> gain familiarity with the PG code base.

> * Ensure that committers don't have to *always* feel guilty about
> not working on other people's patches instead of their own.  Otherwise
> we'd just stay in CF mode all the time.

Oh, yes, I think both of these belong.


> > Submitting a patch as a commitfest entry to a specific commitfest
> > implies a statement by the author that the patch needs input from
> > others. That input can be agreement on design decisions, high level code
> > review, testing, etc.
> 
> ... or even just that the author would like somebody to commit it.

Oh, right ;)


> Also, there's at least one rule you forgot to cover, concerning
> asking people to review patches more or less proportionally to the
> amount of patches they've submitted.  This is surely a lot squishier
> than the other rules, but without it, nothing much happens.

Agreed.

I think we actually should be much more aggressive about it too.

Greetings,

Andres Freund