On Thu, May 16, 2024 at 5:46 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Right, so what can we do about that?  Does Needs Review state need to
> be subdivided, and if so how?

It doesn't really matter how many states we have if they're not set accurately.

> At this point it seems like there's consensus to have a "parking"
> section of the CF app, separate from the time-boxed CFs, and I hope
> somebody will go make that happen.  But I don't think that's our only
> issue, so we need to keep thinking about what should be improved.

I do *emphatically* think we need a parking lot. And a better
integration between commitfest.postgresql.org and the cfbot, too. It's
just ridiculous that more work hasn't been put into this. I'm not
faulting Thomas or anyone else -- I mean, it's not his job to build
the infrastructure the project needs any more than it is anyone else's
-- but for a project of the size and importance of PostgreSQL to take
years to make minor improvements to this kind of critical
infrastructure is kind of nuts. If we don't have enough volunteers,
let's go recruit some more and promise them cookies.

I think you're overestimating the extent to which the problem is "we
don't reject enough patches". That *is* a problem, but it seems we
have also started rejecting some patches that just never really got
much attention for no particularly good reason, while letting other
things linger that didn't really get that much more attention, or
which were objectively much worse ideas. I think that one place where
the process is breaking down is in the tacit assumption that the
person who wrote the patch wants to get it committed. In some cases,
people park things in the CF for CI runs without a strong intention of
pursuing them; in other cases, the patch author is in no kind of rush.

I think we need to move to a system where patches exist independent of
a CommitFest, and get CI run on them unless they get explicitly closed
or are too inactive or some other criterion that boils down to nobody
cares any more. Then, we need to get whatever subset of those patches
need to be reviewed in a particular CommitFest added to that
CommitFest. For example, imagine that the CommitFest is FORCIBLY empty
until a week before it starts. You can still register patches in the
system generally, but that just means they get CI runs, not that
they're scheduled to be reviewed. A week before the CommitFest,
everyone who has a patch registered in the system that still applies
gets an email saying "click here if you think this patch should be
reviewed in the upcoming CommitFest -- if you don't care about the
patch any more or it needs more work before other people review it,
don't click here". Then, the CommitFest ends up containing only the
things where the patch author clicked there during that week.

For bonus points, suppose we make it so that when you click the link,
it takes you to a box where you can type in a text comment that will
display in the app, explaining why your patch needs review: "Robert
says the wire protocol changes in this patch are wrong, but I think
he's full of hot garbage and want a second opinion!" (a purely
hypothetical example, I'm sure) If you put in a comment like this when
you register your patch for the CommitFest, it gets a sparkly gold
border and floats to the top of the list, or we mail you a Kit-Kat
bar, or something. I don't know.

The point is - we need a much better signal to noise ratio here. I bet
the number of patches in the CommitFest that actually need review is
something like 25% of the total. The rest are things that are just
parked there by a committer, or that the author doesn't care about
right now, or that are already being actively discussed, or where
there's not a clear way forward. We could create new statuses for all
of those states - "Parked", "In Hibernation," "Under Discussion," and
"Unclear" - but I think that's missing the point. What we really want
is to not see that stuff in the first place. It's a CommitFest, not
once-upon-a-time-I-wrote-a-patch-Fest.

-- 
Robert Haas
EDB: http://www.enterprisedb.com


Reply via email to