Re: Commitfest Update - Mailing list pgsql-hackers

Justin,

(Consolidating replies here.)

On 7/15/22 19:13, Justin Pryzby wrote:
> cfbot is Thomas's project, so moving it run on postgres vm was one step, but I
> imagine the "integration with cfapp" requires coordination with Magnus.
> 
> What patch ?

https://www.postgresql.org/message-id/CAAWbhmg84OsO5VkaSjX4jokHy8mdpWpNKFgZJHHbb4mprXmtiQ%40mail.gmail.com

It was intended to be a toe in the water -- see if I'm following
conventions, and if I even have the right list.

>>> Similarly, patches could be summarily set to "waiting on author" if they didn't
>>> recently apply, compile, and pass tests.  That's the minimum standard.
>>> However, I think it's better not to do this immediately after the patch stops
>>> applying/compiling/failing tests, since it's usually easy enough to review it.
>>
>> It's hard to argue with that, but without automation, this is plenty of
>> busy work too.
> 
> I don't think that's busywork, since it's understood to require human
> judgement, like 1) to deal with false-positive test failures, and 2) check if
> there's actually anything left for the author to do; 3) check if it passed
> tests recently; 4) evaluate existing opinions in the thread and make a
> judgement call.

[Dev hat; strong opinions ahead.]

I maintain that 1) and 3) are busy work. You should not have to do those
things, in the ideal end state.

>> I think this may have been the goal but I don't think it actually works
>> in practice. The app refuses to let you carry a RwF patch to a new CF.
> 
> I was able to do what Peter said.  I don't know why the cfapp has that
> restriction, it seems like an artificial constraint.
Thanks, I'll work on a patch.

> On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
> I'm not suggesting to give the community regulars special treatment, but you
> could reasonably assume that when they added themselves and then "didn't remove
> themself", it was on purpose and not by omission.  I think most of those people
> would be surprised to learn that they're no longer considered to be reviewing
> the patch.

For some people, I can maybe(?) assume that, but I'm being honest when I
say that I don't really know who that's true for. I definitely don't
think it's true for everybody. And once I start making those decisions
as a CFM, then it really does boil down to who I know and have
interacted with before.

> Can you give an example of a patch where you sent a significant review, and
> added yourself as a reviewer, but wouldn't mind if someone summarily removed
> you, in batch ?

Literally all of them. That's probably the key disconnect here, and why
I didn't think too hard about doing it. (I promise, I didn't think to
myself "I would really hate it if someone did this to me", and then go
ahead and do it to twenty-some other people. :D)

I come from OSS communities that discourage cookie-licking, whether
accidental or on purpose. I don't like marking myself as a Reviewer in
general (although I have done it, because it seems like the thing to do
here?). Simultaneous reviews are never "wasted work" and I'd just rather
not call dibs on a patch. So I wouldn't have a problem with someone
coming along, seeing that I haven't interacted with a patch for a while,
and removing my name. I trust that committers will give credit if credit
is due.

> The stated goal was to avoid the scenario that a would-be reviewer decides not
> to review a patch because cfapp already shows someone else as a reviewer.  I'm
> sure that could happen, but I doubt it's something that happens frequently..

I do that every commitfest. It's one of the first things I look at to
determine what to pay attention to, because I'm trying to find patches
that have slipped through the cracks. As Tom pointed out, others do it
too, though I don't know how many or if their motivations match mine.

>> Why do you think it's useful to remove annotations that people added ? (And, if
>> it were useful, why shouldn't that be implemented in the cfapp, which already
>> has all the needed information.)
> 
> Or, to say it differently, since "reviewers" are preserved when a patch is
> moved to the next CF, it comes as a surprise when by some other mechanism or
> policy the field doesn't stay there.  (If it's intended to be more like a
> per-CF field, I think its behavior should be changed in the cfapp, to avoid
> manual effort, and to avoid other people executing it differently.)

It was my assumption, based on the upthread discussion, that that was
the end goal, and that we just hadn't implemented it yet for lack of time.

>> It undoes work that you and others have done to make the historical
>> record more accurate, and I think that's understandably frustrating. But
>> I thought we were trying to move away from that usage of it altogether.
> 
> I gather that your goal was to make the "reviewers" field more like "people who
> are reviewing the current version of the patch", to make it easy to
> find/isolate patch-versions which need to be reviewed, and hopefully accelarate
> the process.

Yes.
> But IMO there's already no trouble finding the list of patches which need to be
> reviewed - it's the long list that say "Needs Review" - which is what's
> actually needed; that's not easy to do, which is why it's a long list, and no
> amount of updating the annotations will help with that.  I doubt many people
> search for patches to review by seeking out those which have no reviewer (which
> is not a short list anyway).

I do, because I'm looking to maximize bang for buck. When people have
asked me as CFM for help finding patches to review, that's one of the
criteria I use. Needs Review is just too broad, and frankly seems to be
overwhelming to new reviewers I've spoken with.

Honestly, what I really want is buckets based on categories of *what you
can actually do for the patch*. "We want this but it's been stuck in
perma-rebase", "this is blocked on something you can't possibly
influence as a reviewer", "this needs review from someone who
understands every internal system", "this has been stale for months and
is on the chopping block". These are all things that seem to pop up
semi-regularly.

> Here's an idea: send out batch mails to people who are listed as reviewers for
> patches which "Need Review".  That starts to treat the reviewers field as a
> functional thing rather than purely an annotation.  Be sure in your message to
> say "You are receiving this message because you're listed as a reviewer for a
> patch which -Needs Review-".  I think it's reasonable to get a message like
> that 1 or 2 times per month (but not per-month-per-patch).  Ideally it'd
> include the list of patches specific to that reviewer, but I think it'd okay to
> get an un-personalized email reminder 1x/month with a link.

I don't think that addresses the goal, or at least not the goal that I
was pursuing when I was pruning this field. Reminding existing reviewers
doesn't help organize the incoming funnel of new reviewers. And while it
might remind the regular contributors to remove their names if they've
gone stale, it won't ease the CFM busywork for people who have
completely gone silent.

> BTW, one bulk update to make is for the few dozen patches that say "v15" on
> them, and (excluding bugfixes) those are nearly all wrong.  Since the field
> isn't visible in cfbot, it's mostly ignored.  The field is useful toward the
> end of a release cycle to indicate patches that aren't intended for
> consideration for the next release.  Ideally, it'd also be used to indicate the
> patches *are* being considered, but it seems like nobody does that and it ends
> up being a surprise which patches are or are not committed (this seems weird
> and easy to avoid but .. ).  The patches which say "v15" are probably from
> patches submitted during the v15 cycle, and now the version should be removed,
> unless there's a reason to believe the patch is going to target v16 (like if a
> committer has assigned themself).

My recent track record makes me reluctant to do any more bulk updates if
I don't really understand how a field is being used. That whole thing
sounds like something we should address with a better workflow.

> Thanks for receiving my criticism well :)

And thank you for speaking up so quickly! It's a lot easier to undo
partial damage :D (Speaking of which: where is that CF update stream you
mentioned?)

--Jacob