Thread: Commitfest: The Good, The Bad, and the Ugly

Commitfest: The Good, The Bad, and the Ugly

From
David Fetter
Date:
Folks,

We're almost half way through the commitfest, and so I'll start with:

The Good:

- Most patches still in play have a reviewer.

- It's possible for one person to post 5 reviews in a day.  Robert
  Haas actually did this on his own time yesterday.

- New people have been reviewing patches, at least up to the
  Submission criteria.

The Bad:

- There is 1 (one) patch marked "Committed" or "Ready for Committer,"
  where neither the author nor reviewer is a committer.  This
  basically means we have approximately one RRReviewer.

The Ugly:

- Patches are not getting even basic QA.

The Bad and the Ugly are fixable, and here's how.

At the moment, we've got 7 basic review criteria
http://wiki.postgresql.org/wiki/Reviewing_a_Patch, 5 of which can be
accomplished with C skills somewhere between 0 and tiny.  These are:

  1. Submission review (skills needed: patch, English comprehension)
  2. Usability review (skills needed: test-fu, ability to find and read spec)
  3. Feature test (skills needed: patch, configure, make, pipe errors to log)
  4. Performance review (skills needed: ability to time performance)
  5. Coding review (skills needed: guideline comparison, experience with portability issues, minor C-reading skills)

I'd like to set as a goal that every patch in this commitfest get
those levels of review.  You do not need C skills[1].  You do not need
to be a genius database engine hacker[2].  You just need to be
diligent and want to move the project ahead.

If you haven't yet, get signed in and start reviewing patches.  Sign
in with your community login, and let's get going :)
https://commitfest.postgresql.org/action/commitfest_view?id=7

In case you were wondering, what I'm doing here is part of step 7.

If you think that getting all outstanding patches through step 5 is
not doable, let me know.  If you think it is, this is your chance to
help make it happen.  Write back either way.

Cheers,
David.

[1] If you do have them, help out with step 6, too.
[2] If you are one, help out with step 6, too.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Commitfest: The Good, The Bad, and the Ugly

From
Itagaki Takahiro
Date:
On Wed, Sep 29, 2010 at 6:03 AM, David Fetter <david@fetter.org> wrote:
> The Good:
> - Most patches still in play have a reviewer.

As far as I remember, there were discussions about the issue
"A patch has a reviewer, but in Needs Review state for several weeks "
in 9.0 development.

Do we have any plans for it? According to the commitfest app, one patch
has only one reviewer at once. A new reviewer might avoid reviewing
a patch that have another reviewer already.

--
Itagaki Takahiro

Re: [RRR] Commitfest: The Good, The Bad, and the Ugly

From
Robert Haas
Date:
On Tue, Sep 28, 2010 at 9:11 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> On Wed, Sep 29, 2010 at 6:03 AM, David Fetter <david@fetter.org> wrote:
>> The Good:
>> - Most patches still in play have a reviewer.
>
> As far as I remember, there were discussions about the issue
> "A patch has a reviewer, but in Needs Review state for several weeks "
> in 9.0 development.
>
> Do we have any plans for it? According to the commitfest app, one patch
> has only one reviewer at once. A new reviewer might avoid reviewing
> a patch that have another reviewer already.

No, the column is very clearly labelled "Reviewers", not "Reviewer".
And we have certainly had patches with more than one person's name in
that field in the past.  The issue is rather that we don't have enough
people reviewing.  We haven't had enough people volunteer to do
reviews to even assign ONE person to each patch, let alone two.  There
are, as of this writing, SEVEN patches that have no reviewer at all.

Of course, several of the committers, including you, me, and Tom, have
been working our way through the patches.  And that is great.  But the
point of the CommitFest process is that everyone is supposed to pitch
in and help out, not just the committers.  That is not happening, and
it's a problem.  This process does not work and will not scale if the
committers are responsible for doing all the work on every patch from
beginning to end.  That has never worked, and the fact that we have a
few more committers now doesn't change that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [RRR] Commitfest: The Good, The Bad, and the Ugly

From
Itagaki Takahiro
Date:
On Wed, Sep 29, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> No, the column is very clearly labelled "Reviewers", not "Reviewer".
> And we have certainly had patches with more than one person's name in
> that field in the past.  The issue is rather that we don't have enough
> people reviewing.  We haven't had enough people volunteer to do
> reviews to even assign ONE person to each patch, let alone two.  There
> are, as of this writing, SEVEN patches that have no reviewer at all.

Some of them might be too difficult to review. For example, replication
or snapshot management requires special skills to review.

I'm worrying about new reviewers hesitate to review a patch that has
a previous reviewer, and then, if they think the remaining patches are
too difficult for them, they would just leave the commitfest page.

--
Itagaki Takahiro

Re: [RRR] Commitfest: The Good, The Bad, and the Ugly

From
Robert Haas
Date:
On Tue, Sep 28, 2010 at 9:33 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> On Wed, Sep 29, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> No, the column is very clearly labelled "Reviewers", not "Reviewer".
>> And we have certainly had patches with more than one person's name in
>> that field in the past.  The issue is rather that we don't have enough
>> people reviewing.  We haven't had enough people volunteer to do
>> reviews to even assign ONE person to each patch, let alone two.  There
>> are, as of this writing, SEVEN patches that have no reviewer at all.
>
> Some of them might be too difficult to review. For example, replication
> or snapshot management requires special skills to review.
>
> I'm worrying about new reviewers hesitate to review a patch that has
> a previous reviewer, and then, if they think the remaining patches are
> too difficult for them, they would just leave the commitfest page.

That's a legitimate concern, but I am not sure how much of a problem
it is in practice.  Most people who become round-robin reviewers are
getting pulled into the process a little more than just stumbling
across the CF page by happenstance, or at least I hope they are.  Not
all patches can benefit from multiple reviewers, but CF managers can
and should encourage multiple reviews of those that can.  However, at
the moment, the problem is that regardless of who is assigned to do
what, we're not getting enough reviews done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company