Обсуждение: [Commitfest 2022-07] Begins Now

Hello!

It's been July everywhere on Earth for a few hours, so the July
commitfest is now in progress:

    https://commitfest.postgresql.org/38/

New patches may be registered for the next commitfest in September.
Pick some patches to review and have fun!

Happy hacking,
--Jacob

On 7/1/22 08:08, Jacob Champion wrote:
> It's been July everywhere on Earth for a few hours, so the July
> commitfest is now in progress:
> 
>     https://commitfest.postgresql.org/38/
One week down, three to go.

I forgot to put the overall status in the last email. We started the
month with the following stats:

    Needs review:         214
    Waiting on Author:     36
    Ready for Committer:   23
    Committed:             21
    Moved to next CF:       1
    Withdrawn:              5
    Rejected:               2
    Returned with Feedback: 3
    --
    Total:                305

And as of this email, we're now at

    Needs review:         193
    Waiting on Author:     38
    Ready for Committer:   24
    Committed:             37
    Moved to next CF:       2
    Withdrawn:              6
    Rejected:               2
    Returned with Feedback: 3
    --
    Total:                305

That's sixteen patchsets committed in the first week.

Have a good weekend,
--Jacob

[changing the subject line, was "[Commitfest 2022-07] Begins Now"]

On Fri, Jul 15, 2022 at 1:37 AM Wenjing Zeng <wjzeng2012@gmail.com> wrote:
> Please move the Global Temporary table to check next month, that is at 202208.
> I need more time to process the existing issue.

Hi Wenjing,

My current understanding is that RwF patches can't be moved (even by
the CFM). You can just reattach the existing thread to a new CF entry
when you're ready, as discussed in [1]. (Reviewers can sign themselves
back up; don't carry them over.)

It does seem annoying to have to reapply annotations, though. I would
like to see the CF app support this and I'll try to put a patch
together sometime (there's a growing list).

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/CALT9ZEGoVd6%2B5FTJ3d6DXRO4z%3DrvqK%2BdjrmgSiFo7dkKeMkyfQ%40mail.gmail.com

On 7/8/22 16:42, Jacob Champion wrote:
> On 7/1/22 08:08, Jacob Champion wrote:
>> It's been July everywhere on Earth for a few hours, so the July
>> commitfest is now in progress:
>>
>>     https://commitfest.postgresql.org/38/

Halfway through!

We are now at

    Needs review:         175
    Waiting on Author:     43
    Ready for Committer:   20
    Committed:             52
    Moved to next CF:       2
    Returned with Feedback: 4
    Rejected:               3
    Withdrawn:              6
    --
    Total:                305

Since last week, that's fifteen more committed patchsets, with the Ready
for Committer queue holding fairly steady. Nice work, everyone.

I started removing stale Reviewers fairly aggressively today, as
discussed in [1], but there was some immediate feedback and I have
paused that process for now. If you're wondering why you are no longer
marked as reviewer on a patch, I followed the following rule: if you
were signed up to review before June of this year, but you haven't
interacted with the patch in this commitfest, I removed you from the
list. If you have thoughts/comments on this approach, please share them!

Thanks,
--Jacob

[1]

https://www.postgresql.org/message-id/flat/34b32cb2-a728-090a-00d5-067305874174%40timescale.com#3247e661b219f8736ae418c9b5452d63

Hi,

On 2022-07-15 16:42:03 -0700, Jacob Champion wrote:
> I started removing stale Reviewers fairly aggressively today, as
> discussed in [1], but there was some immediate feedback and I have
> paused that process for now. If you're wondering why you are no longer
> marked as reviewer on a patch, I followed the following rule: if you
> were signed up to review before June of this year, but you haven't
> interacted with the patch in this commitfest, I removed you from the
> list. If you have thoughts/comments on this approach, please share them!

I'd make it dependent on whether there have been previous rounds of feedback
or not. If somebody spent a good amount of time reviewing a patch previously,
but then didn't review the newest version in the last few weeks, it doesn't
seem useful to remove them from the CF entry. The situation is different if
somebody has signed up but not done much.

Greetings,

Andres Freund

On 7/15/22 16:51, Andres Freund wrote:
> I'd make it dependent on whether there have been previous rounds of feedback
> or not. If somebody spent a good amount of time reviewing a patch previously,
> but then didn't review the newest version in the last few weeks, it doesn't
> seem useful to remove them from the CF entry. The situation is different if
> somebody has signed up but not done much.

If someone put a lot of review into a patchset a few months ago, they
absolutely deserve credit. But if that entry has been sitting with no
feedback this month, why is it useful to keep that Reviewer around?

(We may want to join this with the other thread.)

--Jacob

Hi,

On 2022-07-15 17:28:06 -0700, Jacob Champion wrote:
> On 7/15/22 16:51, Andres Freund wrote:
> > I'd make it dependent on whether there have been previous rounds of feedback
> > or not. If somebody spent a good amount of time reviewing a patch previously,
> > but then didn't review the newest version in the last few weeks, it doesn't
> > seem useful to remove them from the CF entry. The situation is different if
> > somebody has signed up but not done much.
> 
> If someone put a lot of review into a patchset a few months ago, they
> absolutely deserve credit. But if that entry has been sitting with no
> feedback this month, why is it useful to keep that Reviewer around?

IDK, I've plenty times given feedback and it took months till it all was
implemented. What's the point of doing further rounds of review until then?

Greetings,

Andres Freund

On 16.07.22 01:16, Jacob Champion wrote:
> My current understanding is that RwF patches can't be moved (even by
> the CFM). You can just reattach the existing thread to a new CF entry
> when you're ready, as discussed in [1]. (Reviewers can sign themselves
> back up; don't carry them over.)

I think you can just change the entry back to "needs review" and then 
move it forward.

Hi hackers,

> > If someone put a lot of review into a patchset a few months ago, they
> > absolutely deserve credit. But if that entry has been sitting with no
> > feedback this month, why is it useful to keep that Reviewer around?

As I recall, several committers reported before that they use
Reviewers field in the CF application when writing the commit message.
I would argue that this is the reason.

-- 
Best regards,
Aleksander Alekseev

On 2022-Jul-18, Aleksander Alekseev wrote:

> Hi hackers,
> 
> > > If someone put a lot of review into a patchset a few months ago, they
> > > absolutely deserve credit. But if that entry has been sitting with no
> > > feedback this month, why is it useful to keep that Reviewer around?
> 
> As I recall, several committers reported before that they use
> Reviewers field in the CF application when writing the commit message.
> I would argue that this is the reason.

Maybe we need two separate reviewer columns -- one for credits
(historical tracking) and one for people currently reviewing a patch.
So we definitely expect an email "soon" from someone in the second
column, but not from somebody who is only in the first column.

-- 
Álvaro Herrera

On 7/15/22 16:42, Jacob Champion wrote:
> If you have thoughts/comments on this approach, please share them!

Okay, plenty of feedback to sift through here.

[CFM hat]

First of all: mea culpa. I unilaterally made a change that I had assumed
would be uncontroversial; it clearly was not, and I interrupted the flow
of the CF for people when my goal was to be mostly invisible this month.
 (My single email to a single thread saying "any objections?" is, in
retrospect, not nearly enough reach or mandate to have made this
change.) Big thank you to Justin for seeing it happen and speaking up
immediately.

Here is a rough summary of opinions that have been shared so far; pulled
from the other thread [1] as well:

There are at least three major use cases for the Reviewer field at the
moment.

1) As a new reviewer, find a patch that needs help moving forward.
2) As a committer, give credit to people who moved the patch forward.
3) As an established reviewer, keep track of patches "in flight."

I had never realized the third case existed. To those of you who I've
interrupted by modifying your checklist without permission, I'm sorry. I
see that several of you have already added yourselves back, which is
great; I will try to find the CF update stream that has been alluded to
elsewhere and see if I can restore the original Reviewers lists that I
nulled out on Friday.

It was suggested that we track historical reviewers and current reviews
separately from each other, to handle both cases 1 and 2.

There appears to be a need for people to be able to consider a patch
"blocked" pending some action, so that further review cycles aren't
burned on it. Some people use Waiting on Author for that, but others use
WoA as soon as an email is sent. The two cases have similarities but, to
me at least, aren't the same and may be working at cross purposes.

It is is apparently possible to pull one of your closed patches from a
prior commitfest into the new one, but you have to set it back to Needs
Review first. I plan to work on a CF patch to streamline that, if
someone does not beat me to it.

Okay, I think those are the broad strokes. I will put my [dev hat] on
now and respond more granularly to threads, with stronger opinions.

Thanks,
--Jacob

[1]

https://www.postgresql.org/message-id/flat/34b32cb2-a728-090a-00d5-067305874174%40timescale.com#3247e661b219f8736ae418c9b5452d63

[dev hat]

On 7/15/22 18:07, Andres Freund wrote:
> IDK, I've plenty times given feedback and it took months till it all was
> implemented. What's the point of doing further rounds of review until then?

I guess I would wonder why we're optimizing for that case. Is it helpful
for that patch to stick around in an active CF for months? There's an
established need for keeping a "TODO item" around and not letting it
fall off, but I think that should remain separate in an application
which seems to be focused on organizing active volunteers.

And if that's supposed to be what Waiting on Author is for, then I think
we need more guidance on how to use that status effectively. Some
reviewers seem to use it as a "replied" flag. I think there's a
meaningful difference between soft-blocked on review feedback and
hard-blocked on new implementation. And maybe there's even a middle
state, where the patch just needs someone to do a mindless rebase.

I think you're in a better position than most to "officially" decide
that a patch can no longer benefit from review. Most of us can't do
that, I imagine -- nor should we.

Thanks,
--Jacob

Hi,

On 2022-07-18 12:22:25 -0700, Jacob Champion wrote:
> [dev hat]
> 
> On 7/15/22 18:07, Andres Freund wrote:
> > IDK, I've plenty times given feedback and it took months till it all was
> > implemented. What's the point of doing further rounds of review until then?
> 
> I guess I would wonder why we're optimizing for that case. Is it helpful
> for that patch to stick around in an active CF for months?

I'm not following - I'm talking about the patch author needing a while to
address the higher level feedback given by a reviewer. The author might put
out a couple new versions, which each might still benefit from review. In that
- pretty common imo - situation I don't think it's useful for the reviewer
that provided the higher level feedback to be removed from the patch.

Greetings,

Andres Freund

On 7/18/22 12:32, Andres Freund wrote:
> I'm not following - I'm talking about the patch author needing a while to
> address the higher level feedback given by a reviewer. The author might put
> out a couple new versions, which each might still benefit from review. In that
> - pretty common imo - situation I don't think it's useful for the reviewer
> that provided the higher level feedback to be removed from the patch.

Okay, I think I get it now. Thanks.

There's still something off in that case that I can't quite
articulate... Is it your intent to use Reviewer as a signal that "I'll
come back to this eventually"? As a signal to other prospective
reviewers that you're handling the patch? How should a CFM move things
forward when they come to a patch that's been responded to by the author
but the sole Reviewer has been silent?

--Jacob

Hi,

On 2022-07-18 13:34:52 -0700, Jacob Champion wrote:
> On 7/18/22 12:32, Andres Freund wrote:
> > I'm not following - I'm talking about the patch author needing a while to
> > address the higher level feedback given by a reviewer. The author might put
> > out a couple new versions, which each might still benefit from review. In that
> > - pretty common imo - situation I don't think it's useful for the reviewer
> > that provided the higher level feedback to be removed from the patch.
> 
> Okay, I think I get it now. Thanks.
> 
> There's still something off in that case that I can't quite
> articulate... Is it your intent to use Reviewer as a signal that "I'll
> come back to this eventually"?

That, and as a way to find out what I possible should look at again.


> As a signal to other prospective reviewers that you're handling the patch?

Definitely not. I think no reviewer on a patch should be taken as
that. There's often many angles to a patch, and leaving trivial patches aside,
no reviewer is an expert in all of them.


> How should a CFM move things forward when they come to a patch that's been
> responded to by the author but the sole Reviewer has been silent?

Ping the reviewer and/or thread, ensure the patch is needs-review state. I
don't think removing reviewers in the CF app would help with that anyway -
often some reviewers explicitly state that they're only reviewing a specific
part of the patch, or that looked at everything but lack expertise to be
confident in their positions etc.  Such reviewers might do more rounds of
feedback to newer patches, but the patch might still need more feedback.

ISTM that you're trying to get patches to have zero reviewers if they need
more reviewers, because that can serve as a signal in the CF app. But to me
that's a bad proxy.

Greetings,

Andres Freund

On 7/18/22 02:53, Alvaro Herrera wrote:
> On 2022-Jul-18, Aleksander Alekseev wrote:
> 
>> Hi hackers,
>>
>>>> If someone put a lot of review into a patchset a few months ago, they
>>>> absolutely deserve credit. But if that entry has been sitting with no
>>>> feedback this month, why is it useful to keep that Reviewer around?
>>
>> As I recall, several committers reported before that they use
>> Reviewers field in the CF application when writing the commit message.
>> I would argue that this is the reason.
> 
> Maybe we need two separate reviewer columns -- one for credits
> (historical tracking) and one for people currently reviewing a patch.
> So we definitely expect an email "soon" from someone in the second
> column, but not from somebody who is only in the first column.

+1


-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 7/15/22 16:42, Jacob Champion wrote:
> On 7/8/22 16:42, Jacob Champion wrote:
>> On 7/1/22 08:08, Jacob Champion wrote:
>>> It's been July everywhere on Earth for a few hours, so the July
>>> commitfest is now in progress:
>>>
>>>     https://commitfest.postgresql.org/38/

With one week remaining, we're now at

    Needs review:         162
    Waiting on Author:     42
    Ready for Committer:   21
    Committed:             60
    Moved to next CF:       2
    Returned with Feedback: 6
    Rejected:               3
    Withdrawn:              9
    --
    Total:                305

An additional eight patches committed and five closed, with the Ready
for Committer queue still steady.

Next week I'll begin highlighting patches for help or closure.

Have a good weekend,
--Jacob

On Mon, Jul 18, 2022 at 1:44 PM Andres Freund <andres@anarazel.de> wrote:
> ISTM that you're trying to get patches to have zero reviewers if they need
> more reviewers, because that can serve as a signal in the CF app. But to me
> that's a bad proxy.

Okay. I need to put some more thought into what it is that I really
want (and the wiki needs to be updated, because it's suggesting that
the CFM use the Reviewers field in this way as well). Thanks for the
feedback!

--Jacob

On 7/22/22 16:03, Jacob Champion wrote:
> On 7/15/22 16:42, Jacob Champion wrote:
>> On 7/8/22 16:42, Jacob Champion wrote:
>>> On 7/1/22 08:08, Jacob Champion wrote:
>>>> It's been July everywhere on Earth for a few hours, so the July
>>>> commitfest is now in progress:
>>>>
>>>>     https://commitfest.postgresql.org/38/

It's the final weekend!

The July CF will officially close after 11:59p, July 31, anywhere on
Earth. (There will be a grace period of a few hours, because, well, I'll
be asleep when the deadline passes.) At that point I'll begin moving
active patches to the next CF, and closing out others as discussed in
the triage threads.

Our statistics are now at

    Needs review:         150
    Waiting on Author:     42
    Ready for Committer:   22
    Committed:             68
    Moved to next CF:       5
    Returned with Feedback: 7
    Rejected:               3
    Withdrawn:             11
    --
    Total:                308

That's an additional eight committed, three closed, and nine Ready for
Committer.

--Jacob