Re: next CommitFest

The next CommitFest is scheduled to start in a week. So far, it
doesn't look too bad, though a lot could change between now and then.

I would personally prefer not to be involved in the management of the
next CommitFest. Having done all of the July CommitFest and a good
chunk of the September CommitFest, I am feeling a bit burned out.

One pretty major fly in the ointment is that neither Hot Standby nor
Streaming Replication has been committed or shows much sign of being
about to be committed. I think this is bad. These are big features
that figure to have some bugs and break some things. If they're not
committed in time for alpha3, then there won't be any significant
testing of these prior to alpha4/beta1, at the earliest. I think
that's likely to lead to either (1) a very long beta period followed
by a late release or (2) a buggy release. I feel like Simon Riggs and
Fujii Masao really pulled out all the stops to get these ready in time
for the September CommitFest, and while I'm not in a hurry to break
the world, I think the sooner these can hit the tree, the better of
we'll be in terms of releasing 8.5.

Just my $0.02,

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I would personally prefer not to be involved in the management of the
> next CommitFest. Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.

You did yeoman work on both --- thanks for that!

Do we have another volunteer to do this for the November fest?

regards, tom lane

*snip*

> One pretty major fly in the ointment is that neither Hot Standby nor
> Streaming Replication has been committed or shows much sign of being
> about to be committed. I think this is bad. These are big features
> that figure to have some bugs and break some things. If they're not
> committed in time for alpha3, then there won't be any significant
> testing of these prior to alpha4/beta1, at the earliest. I think
> that's likely to lead to either (1) a very long beta period followed
> by a late release or (2) a buggy release. I feel like Simon Riggs and
> Fujii Masao really pulled out all the stops to get these ready in time
> for the September CommitFest, and while I'm not in a hurry to break
> the world, I think the sooner these can hit the tree, the better of
> we'll be in terms of releasing 8.5.
>
> Just my $0.02,
>
>

absolutely, we should be commit this.
we did some testing and things look stable.
also, people would most likely want to build code on top of it in be
ready for 8.5 (support scripts, etc.). this is important in order to
create some acceptance in "user land".
this stuffs seems mature and very well thought.

just my $0.02 ...

regards,

hans-jürgen schönig

--
Cybertec Schoenig & Schoenig GmbH
Reyergasse 9 / 2
A-2700 Wiener Neustadt
Web: www.postgresql-support.de

> I would personally prefer not to be involved in the management of the
> next CommitFest. Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.

Dave, Selena and I will all be in Japan during the first week of the CF.
I can help after that, but during might be hard. Who else helped with
the last CF? Is someone else ready to be a CF helper?

--Josh Berkus

On Sun, 8 Nov 2009, Robert Haas wrote:

> I would personally prefer not to be involved in the management of the
> next CommitFest. Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.

I was just poking around on the Wiki, and it looks like the role of the
CommitFest manager isn't very well documented yet. Since you've done all
of them since introducing the new CF software, I'm not sure if anyone else
even knows exactly what you've been doing. The transition over to that
was so successful there isn't even a copy of the schedule for 8.5 on the
Wiki itself. Could you find some time this week to rattle off an outline
of the work involved? It's hard to decide whether to volunteer to help
without having a better idea of what's required.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Hi!

On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
> On Sun, 8 Nov 2009, Robert Haas wrote:
>
>> I would personally prefer not to be involved in the management of the
>> next CommitFest.  Having done all of the July CommitFest and a good
>> chunk of the September CommitFest, I am feeling a bit burned out.
>
> I was just poking around on the Wiki, and it looks like the role of the
> CommitFest manager isn't very well documented yet.  Since you've done all of
> them since introducing the new CF software, I'm not sure if anyone else even
> knows exactly what you've been doing.  The transition over to that was so
> successful there isn't even a copy of the schedule for 8.5 on the Wiki
> itself.  Could you find some time this week to rattle off an outline of the
> work involved?  It's hard to decide whether to volunteer to help without
> having a better idea of what's required.

It's pretty straightforward. Robert has actually done a great job of
communicating about this to the patch reviewers.

I'd be happy to get together at some pre-appointed hour this weekend
(Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already
planning to get together to review some patches this Sunday from 3-5pm
PST, so that is a convenient time for me.

I can also help with commitfest admin tasks, but not in a dedicated
way until after Thanksgiving as I'm going to be be traveling or on
vacation.

-selena

--
http://chesnok.com/daily - me
http://endpoint.com - work

Selena Deckelmann wrote:
> Hi!
>
> On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
>> On Sun, 8 Nov 2009, Robert Haas wrote:
>>
>>> I would personally prefer not to be involved in the management of the
>>> next CommitFest. Having done all of the July CommitFest and a good
>>> chunk of the September CommitFest, I am feeling a bit burned out.
>> I was just poking around on the Wiki, and it looks like the role of the
>> CommitFest manager isn't very well documented yet. Since you've done all of
>> them since introducing the new CF software, I'm not sure if anyone else even
>> knows exactly what you've been doing. The transition over to that was so
>> successful there isn't even a copy of the schedule for 8.5 on the Wiki
>> itself. Could you find some time this week to rattle off an outline of the
>> work involved? It's hard to decide whether to volunteer to help without
>> having a better idea of what's required.
>
> It's pretty straightforward. Robert has actually done a great job of
> communicating about this to the patch reviewers.

agreed

>
> I'd be happy to get together at some pre-appointed hour this weekend
> (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already
> planning to get together to review some patches this Sunday from 3-5pm
> PST, so that is a convenient time for me.
>
> I can also help with commitfest admin tasks, but not in a dedicated
> way until after Thanksgiving as I'm going to be be traveling or on
> vacation.

I would be interested in helping out as well but I won't be able to
dedicate a lot of time before that timeframe as well :(

Stefan

On Sun, Nov 8, 2009 at 8:52 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> The next CommitFest is scheduled to start in a week.  So far, it
> doesn't look too bad, though a lot could change between now and then.
>
> I would personally prefer not to be involved in the management of the
> next CommitFest.  Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.
>

why we need a full time manager at all?
why not simply use -rrreviewers to track the status of a patch? of
course, we hope the author or reviewer to change the status as
appropiate but we have seen many people including missing discussions
and changing the status of hanging patches...

i guess we can make the commitfest app send an email stating that
reviewer_foo is taking the patch bar, and maybe sending emails after
some days if nothing has happened with that patch... and an email
every week or every few days saying how many patches are, how many are
being reviewed, how many hasn't been reviewed, and so on...

then the remaining work should be not that much, no?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova
<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> why we need a full time manager at all?
> why not simply use -rrreviewers to track the status of a patch? of
> course, we hope the author or reviewer to change the status as
> appropiate but we have seen many people including missing discussions
> and changing the status of hanging patches...

Well, actually, that's precisely were I've been putting in a ton of
work - making sure patches aren't left hanging. If reviewers would do
more of that work, this process would run a lot smoother and be much
less onerous for the CM.

...Robert

Selena,

> I'd be happy to get together at some pre-appointed hour this weekend
> (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already
> planning to get together to review some patches this Sunday from 3-5pm
> PST, so that is a convenient time for me.

Aren't you running OpenSQL this weekend?

--Josh

On Wed, Nov 11, 2009 at 4:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova
> <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>> why we need a full time manager at all?
>> why not simply use -rrreviewers to track the status of a patch? of
>> course, we hope the author or reviewer to change the status as
>> appropiate but we have seen many people including missing discussions
>> and changing the status of hanging patches...
>
> Well, actually, that's precisely were I've been putting in a ton of
> work - making sure patches aren't left hanging.

that's why i guess sending automatic mails would be a good way to
remember that a reviewer had a patch in their control or to tell
reviewers/committers there are still patches for review/commit

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Selena Deckelmann wrote:
> On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
>
>> I was just poking around on the Wiki, and it looks like the role of the
>> CommitFest manager isn't very well documented yet.
>>
>
> It's pretty straightforward. Robert has actually done a great job of
> communicating about this to the patch reviewers.
>
That's good to hear. What I was hinting at was that some of the
community knowledge here should start getting written down now that the
process has matured, rather than trying to directly transfer just to one
other person. I'm not sure if Robert has shared 100% of what he does
with the reviewers or not, but in general the easiest way to divest
yourself of a position is to document how someone else can do it. I
don't know that having to poke through list archives or chat with
someone is necessarily the best way to transfer that knowledge.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Wed, Nov 11, 2009 at 5:06 PM, Jaime Casanova
<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> On Wed, Nov 11, 2009 at 4:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova
>> <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>>> why we need a full time manager at all?
>>> why not simply use -rrreviewers to track the status of a patch? of
>>> course, we hope the author or reviewer to change the status as
>>> appropiate but we have seen many people including missing discussions
>>> and changing the status of hanging patches...
>>
>> Well, actually, that's precisely were I've been putting in a ton of
>> work - making sure patches aren't left hanging.
>
> that's why i guess sending automatic mails would be a good way to
> remember that a reviewer had a patch in their control or to tell
> reviewers/committers there are still patches for review/commit

I think an automatic system would probably not be too valuable, but
you're welcome to submit a patch against commitfest.postgresql.org
(source code is published at git.postgresql.org). I'd recommend
proposing a design on -hackers first.

It's easy to generate systems that spew out a lot of email, but the
system doesn't really have any understanding of what is really going
on. When I send out emails to nag people, I actually put quite a bit
of thought into what I say. Sometimes I try to summarize the current
status of the patch, sometimes I add my own thoughts, sometimes I just
fire off a one-liner. I think that adds value, but perhaps I
overestimate myself.

...Robert

On Wed, Nov 11, 2009 at 5:14 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> Selena Deckelmann wrote:
>
> On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
>
>
> I was just poking around on the Wiki, and it looks like the role of the
> CommitFest manager isn't very well documented yet.
>
>
> It's pretty straightforward. Robert has actually done a great job of
> communicating about this to the patch reviewers.
>
>
> That's good to hear.  What I was hinting at was that some of the community
> knowledge here should start getting written down now that the process has
> matured, rather than trying to directly transfer just to one other person.
> I'm not sure if Robert has shared 100% of what he does with the reviewers or
> not, but in general the easiest way to divest yourself of a position is to
> document how someone else can do it.  I don't know that having to poke
> through list archives or chat with someone is necessarily the best way to
> transfer that knowledge.

I'll try to write something up. Stand by.

...Robert

Robert Haas escreveu:
> I think an automatic system would probably not be too valuable
>
I have the same impression.

> It's easy to generate systems that spew out a lot of email, but the
> system doesn't really have any understanding of what is really going
> on. When I send out emails to nag people, I actually put quite a bit
> of thought into what I say. Sometimes I try to summarize the current
> status of the patch, sometimes I add my own thoughts, sometimes I just
> fire off a one-liner. I think that adds value, but perhaps I
> overestimate myself.
>
But I think we should try to have multiple CMs so we don't burn out someone
else. That group (a small one) could do the same job Robert has been done
(AFAICS a very good job) but in a distributed way. I certainly could be a CM
if I don't have to dedicate too much time at the CF month. I don't know if it
would be as effective as it has been done but it will take that weight off our
CM's shoulders.

--
Euler Taveira de Oliveira
http://www.timbira.com/

On Wed, Nov 11, 2009 at 5:27 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Nov 11, 2009 at 5:14 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>> Selena Deckelmann wrote:
>>
>> On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
>>
>>
>> I was just poking around on the Wiki, and it looks like the role of the
>> CommitFest manager isn't very well documented yet.
>>
>>
>> It's pretty straightforward. Robert has actually done a great job of
>> communicating about this to the patch reviewers.
>>
>>
>> That's good to hear.  What I was hinting at was that some of the community
>> knowledge here should start getting written down now that the process has
>> matured, rather than trying to directly transfer just to one other person.
>> I'm not sure if Robert has shared 100% of what he does with the reviewers or
>> not, but in general the easiest way to divest yourself of a position is to
>> document how someone else can do it.  I don't know that having to poke
>> through list archives or chat with someone is necessarily the best way to
>> transfer that knowledge.
>
> I'll try to write something up.  Stand by.

Here's an attempt.

http://wiki.postgresql.org/wiki/Running_a_CommitFest

...Robert

Robert Haas wrote:
> Here's an attempt.
> http://wiki.postgresql.org/wiki/Running_a_CommitFest
>
Perfect, that's the sort of thing I was looking for the other day but
couldn't find anywhere. I just made a pass through better wiki-fying
that and linking it to the related pages in this area.

Two things look to be true at the moment:

1) The call for reviewers is already running late and needs to start ASAP.
2) Some of the experienced helpers from the previous CFs, like Selena,
should eventually be able to help, just everybody is busy during when
the first round of action has to happen here.

Given all that, I'm thinking that unless we get an enthusiastic
volunteer by tomorrow, I'll kick off the call for reviewers myself and
follow that through to initial patch assignments. I don't expect to
have as much time as Robert put into the last couple of CommitFests
after that, but this one looks smaller and with more familiar patches
than those.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Sun, 2009-11-08 at 20:52 -0500, Robert Haas wrote:

> I would personally prefer not to be involved in the management of the
> next CommitFest. Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.

You did a grand job and everybody appreciates it.

> I feel like Simon Riggs and
> Fujii Masao really pulled out all the stops to get these ready in time
> for the September CommitFest, and while I'm not in a hurry to break
> the world, I think the sooner these can hit the tree, the better of
> we'll be in terms of releasing 8.5.

Sprinting is hard and we all need to rest afterwards.

How about we just slow the pace down a little? Nobody wants you to quit,
we just need to set a sustainable pace.

Looking at the submissions so far, it seems you've done such a grand job
at clearing the backlog that there are few patches in the next fest.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Nov 12, 2009 at 5:04 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Sun, 2009-11-08 at 20:52 -0500, Robert Haas wrote:
>
>> I would personally prefer not to be involved in the management of the
>> next CommitFest.  Having done all of the July CommitFest and a good
>> chunk of the September CommitFest, I am feeling a bit burned out.
>
> You did a grand job and everybody appreciates it.

Thanks.

>> I feel like Simon Riggs and
>> Fujii Masao really pulled out all the stops to get these ready in time
>> for the September CommitFest, and while I'm not in a hurry to break
>> the world, I think the sooner these can hit the tree, the better of
>> we'll be in terms of releasing 8.5.
>
> Sprinting is hard and we all need to rest afterwards.
>
> How about we just slow the pace down a little? Nobody wants you to quit,
> we just need to set a sustainable pace.

I'm not sure exactly what point you're aiming at here, so I'll respond
with a few thoughts that may or may not pertain.

I think it would be really, really good if we could make this release
come out on the schedule previously discussed. 8.4 slipped quite a
bit for reasons that were, IMHO, quite preventable: and, worse, it's
not clear that the slippage really bought us anything, because we
still ended up with a bunch of embarassing bugs. Having said that,
I'm not capable of single-handedly effecting an on-time release, and I
don't particularly want to. In a community where people can disappear
or change roles in the snap of a finger, it's bad to be relying on any
one person to do too much. We need larger, more robust pools of
committers, reviewers, commitfest managers, etc.

Perhaps for next release we should consider spacing the CommitFests
out a little more. I think one CommitFest every 2 months is a little
too tight a schedule. As Peter and others have mentioned previously,
it doesn't leave a lot of time to work on your own patches (behold the
lack of any of my patches in this CommitFest). I think a CommitFest
every 3 months would be too long, but maybe something in the middle.
The trick is to navigate around major holidays while ending around the
right time. Possibly the amount of time between CommitFests doesn't
even need to be constant throughout the release cycle - maybe shorter
at the beginning and longer towards the end.

> Looking at the submissions so far, it seems you've done such a grand job
> at clearing the backlog that there are few patches in the next fest.

Thanks for your kind words. It does seem that most of the major
patches we've seen so far landed last CommitFest, with the exception
of HS and SR. That's not all me, of course - among other people, Tom
did a tremendous amount of work - but I'm glad I was able to help move
it along.

...Robert

Hi,

On Thursday 12 November 2009 12:46:46 Robert Haas wrote:
> Perhaps for next release we should consider spacing the CommitFests
> out a little more.
That may lead to quite a bit frustration on the contributor side though. It
can be very frustrating to have no input for a even longer timeframe...

Andres

On Thu, Nov 12, 2009 at 6:49 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On Thursday 12 November 2009 12:46:46 Robert Haas wrote:
>> Perhaps for next release we should consider spacing the CommitFests
>> out a little more.
> That may lead to quite a bit frustration on the contributor side though. It
> can be very frustrating to have no input for a even longer timeframe...

Yep, that's the flip side. But you'll notice I was suggesting
extending it by a pretty small amount. Anyway, it was just a thought.

...Robert

On Thu, 2009-11-12 at 06:46 -0500, Robert Haas wrote:

> Having said that,
> I'm not capable of single-handedly effecting an on-time release

You're bloody good and the task needs to fit our capability anyway.

So, yes, you are.

> We need larger, more robust pools of
> committers, reviewers, commitfest managers, etc.

We're living in a desert. We just need to remember it. Plan hard, focus
on the important and be real. Move at a smooth pace to save resources.
Don't give up when the going gets tough, just rest up and then continue.

Not a new idea, but I think we should require all patch submitters to do
one review per submission. There needs to be a balance between time
spent on review and time spent on dev. The only real way this happens in
any community is by peer review.

All patch submitters need to know that they must also take their turn as
patch reviewers. If it is a hard rule, then patch *sponsors* would be
forced to accept that they must *also* pay for review time. It is the
sponsors that need to be forced to accept that reality, though we can
only "get at them" through controlling developer behaviour. So, I
propose that we simply ignore patches from developers until they have
done sufficient review to be allowed to develop again.

--
Simon Riggs www.2ndQuadrant.com

On Wed, Nov 11, 2009 at 2:03 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

>> I'd be happy to get together at some pre-appointed hour this weekend
>> (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already
>> planning to get together to review some patches this Sunday from 3-5pm
>> PST, so that is a convenient time for me.
>
> Aren't you running OpenSQL this weekend?

Yes :) But we have lots of volunteers. It's an unconference, and I delegated.

-selena

--
http://chesnok.com/daily - me
http://endpoint.com - work

On Thu, Nov 12, 2009 at 11:31 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Not a new idea, but I think we should require all patch submitters to do
> one review per submission. There needs to be a balance between time
> spent on review and time spent on dev. The only real way this happens in
> any community is by peer review.
>
> All patch submitters need to know that they must also take their turn as
> patch reviewers. If it is a hard rule, then patch *sponsors* would be
> forced to accept that they must *also* pay for review time. It is the
> sponsors that need to be forced to accept that reality, though we can
> only "get at them" through controlling developer behaviour. So, I
> propose that we simply ignore patches from developers until they have
> done sufficient review to be allowed to develop again.

I agree. I would quibble only with the details. I think we should
require patch authors to act as a reviewer for any CommitFest for
which they have submitted patches. We don't need every patch author
to review as many patches as they submit, because some people will
review extras, and some non-patch-authors will review. If they review
one patch each, that's probably more than enough. It's also easier
for bookkeeping purposes.

...Robert

On Thu, Nov 12, 2009 at 8:31 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Not a new idea, but I think we should require all patch submitters to do
> one review per submission. There needs to be a balance between time
> spent on review and time spent on dev. The only real way this happens in
> any community is by peer review.

I like this idea. Perhaps also if a person is reviewing a patch for
the first time, we could make some effort to get a more experienced
person paired up with them.

Pairing up was really useful for the PDXPUG folks working on review.

--
http://chesnok.com/daily - me
http://endpoint.com - work

Simon Riggs escreveu:
> So, I
> propose that we simply ignore patches from developers until they have
> done sufficient review to be allowed to develop again.
>
Is it really impolite for a first-contributor, no?

--
Euler Taveira de Oliveira
http://www.timbira.com/

A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure:
> Simon Riggs escreveu:
> > So, I
> > propose that we simply ignore patches from developers until they have
> > done sufficient review to be allowed to develop again.
>
> Is it really impolite for a first-contributor, no?
>

I don't think so, as long as it's properly explained.

--
Albert Cervera i Areny
http://www.NaN-tic.com
Mòbil: +34 669 40 40 18

> I like this idea. Perhaps also if a person is reviewing a patch for
> the first time, we could make some effort to get a more experienced
> person paired up with them.

When I was CFM last year, I assigned patches for review first if the
patch author was doing a review themselves. Patches whose authors were
not doing reviews often got reviewed last, which means that sometimes
they waited a commitfest.

Of course, Simon submitted so many patches that despite him doing
reviews, it was hard to get them all assigned. Simon: slow down a
little! ;-)

--Josh Berkus

On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny
<albert(at)nan-tic(dot)com> wrote:
> A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure:
>> Simon Riggs escreveu:
>> > So, I
>> > propose that we simply ignore patches from developers until they have
>> > done sufficient review to be allowed to develop again.
>>
>> Is it really impolite for a first-contributor, no?
>>
>
> I don't think so, as long as it's properly explained.

Personally, I would not propose to impose this rule of first-time
contributors, or even second-time contributors. But by about patch #3
I think everyone should be pitching in.

Just MHO, of course.

...Robert

2009/11/13 Euler Taveira de Oliveira <euler(at)timbira(dot)com>:
> Simon Riggs escreveu:
>> So, I
>> propose that we simply ignore patches from developers until they have
>> done sufficient review to be allowed to develop again.
>>
> Is it really impolite for a first-contributor, no?
>

I support Simon's proposal, but I think we would certainly need to
make an exception for first-time contributors.

Cheers,
BJ

On Thu, 2009-11-12 at 15:52 -0200, Euler Taveira de Oliveira wrote:
> Simon Riggs escreveu:
> > So, I
> > propose that we simply ignore patches from developers until they have
> > done sufficient review to be allowed to develop again.
>
> Is it really impolite for a first-contributor, no?

I believe the community has always given extra help to first-time
contributors.

If anyone sees a new contributor being ignored, they should make an
effort to help.

Regards,
Jeff Davis

Robert Haas escribió:
> On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny
> <albert(at)nan-tic(dot)com> wrote:
> > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure:
> >> Simon Riggs escreveu:
> >> > So, I
> >> > propose that we simply ignore patches from developers until they have
> >> > done sufficient review to be allowed to develop again.
> >>
> >> Is it really impolite for a first-contributor, no?
> >
> > I don't think so, as long as it's properly explained.
>
> Personally, I would not propose to impose this rule of first-time
> contributors, or even second-time contributors. But by about patch #3
> I think everyone should be pitching in.

+1. It's OK to not review anything if you're just doing a one-off. If
you submit a bunch of patches you start becoming part of the community.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Thu, 2009-11-12 at 11:36 -0500, Robert Haas wrote:
> I agree. I would quibble only with the details. I think we should
> require patch authors to act as a reviewer for any CommitFest for
> which they have submitted patches. We don't need every patch author
> to review as many patches as they submit, because some people will
> review extras, and some non-patch-authors will review. If they review
> one patch each, that's probably more than enough. It's also easier
> for bookkeeping purposes.

Not all contributors are fluent English readers and writers.

I certainly do not discourage those people from reviewing, but I can
understand how it might be more frustrating and less productive for
them. An important part of review is to read the relevant mailing list
threads and try to tie up loose ends and find a consensus.

Regards,
Jeff Davis

On Thu, Nov 12, 2009 at 1:45 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Thu, 2009-11-12 at 11:36 -0500, Robert Haas wrote:
>> I agree.  I would quibble only with the details.  I think we should
>> require patch authors to act as a reviewer for any CommitFest for
>> which they have submitted patches.  We don't need every patch author
>> to review as many patches as they submit, because some people will
>> review extras, and some non-patch-authors will review.  If they review
>> one patch each, that's probably more than enough.  It's also easier
>> for bookkeeping purposes.
>
> Not all contributors are fluent English readers and writers.
>
> I certainly do not discourage those people from reviewing, but I can
> understand how it might be more frustrating and less productive for
> them. An important part of review is to read the relevant mailing list
> threads and try to tie up loose ends and find a consensus.

Unfortunately, those people's patches also typically require more work
from other community members. Discussions are longer and more
confused, and someone has to rewrite the documentation and comments
before commit. If anything, people whose patches require more help
need to find ways to contribute MORE to the community, not less. I
understand that's not easy, but it's necessary.

...Robert

On Thu, 2009-11-12 at 14:43 -0500, Robert Haas wrote:
> > Not all contributors are fluent English readers and writers.
> >
> > I certainly do not discourage those people from reviewing, but I can
> > understand how it might be more frustrating and less productive for
> > them. An important part of review is to read the relevant mailing list
> > threads and try to tie up loose ends and find a consensus.
>
> Unfortunately, those people's patches also typically require more work
> from other community members. Discussions are longer and more
> confused, and someone has to rewrite the documentation and comments
> before commit. If anything, people whose patches require more help
> need to find ways to contribute MORE to the community, not less. I
> understand that's not easy, but it's necessary.

Keep in mind that many of these people may also be regional contacts,
translators, local conference (or user group) organizers, and provide
support to users in languages other than English. They may even organize
development efforts in languages other than English.

I think the best policies encourage people to help in the ways that they
are most effective, and/or enjoy the most. I know that's a general
statement, and it doesn't translate (excuse the pun) easily into
policy.

I don't have a good answer for that. But sometimes the policy
discussions here seem a little abstract, and I have a hard time seeing
how they apply in real situations. It seems like you have a few
"freeloaders" in mind, but I have a hard time imagining it's more than a
couple people. Maybe they just need a nice reminder to be a more helpful
community member if they want timely feedback on their work?

Regards,
Jeff Davis

On Thu, Nov 12, 2009 at 3:12 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Thu, 2009-11-12 at 14:43 -0500, Robert Haas wrote:
>> > Not all contributors are fluent English readers and writers.
>> >
>> > I certainly do not discourage those people from reviewing, but I can
>> > understand how it might be more frustrating and less productive for
>> > them. An important part of review is to read the relevant mailing list
>> > threads and try to tie up loose ends and find a consensus.
>>
>> Unfortunately, those people's patches also typically require more work
>> from other community members.  Discussions are longer and more
>> confused, and someone has to rewrite the documentation and comments
>> before commit.  If anything, people whose patches require more help
>> need to find ways to contribute MORE to the community, not less.  I
>> understand that's not easy, but it's necessary.
>
> Keep in mind that many of these people may also be regional contacts,
> translators, local conference (or user group) organizers, and provide
> support to users in languages other than English. They may even organize
> development efforts in languages other than English.

They may, but it's beyond my power to keep track of everything that a
person may or may not do. If we're going to have any chance of
enforcing a policy, it has to be simple and clear.

> I don't have a good answer for that. But sometimes the policy
> discussions here seem a little abstract, and I have a hard time seeing
> how they apply in real situations. It seems like you have a few
> "freeloaders" in mind, but I have a hard time imagining it's more than a
> couple people. Maybe they just need a nice reminder to be a more helpful
> community member if they want timely feedback on their work?

Well, you can go back and look at the list of people who had patches
reviewed last CF (see commitfest.postgresql.org), and then go back and
look at the reviewers (see pgsql-rrreviewers archives). It's all
public data. It's not 1 or 2 people - I count at least 8. I could
cross-reference for you and list the names, but that seems like
embarassing those people without accomplishing anything useful.

...Robert

Robert Haas wrote:
> On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny
> <albert(at)nan-tic(dot)com> wrote:
> > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure:
> >> Simon Riggs escreveu:
> >> > So, I
> >> > propose that we simply ignore patches from developers until they have
> >> > done sufficient review to be allowed to develop again.
> >>
> >> Is it really impolite for a first-contributor, no?
> >>
> >
> > I don't think so, as long as it's properly explained.
>
> Personally, I would not propose to impose this rule of first-time
> contributors, or even second-time contributors. But by about patch #3
> I think everyone should be pitching in.

I hate to ask, but how would we enforce this? Do we no longer apply
patches for 3rd-time submitters who have not reviewed? That seems to be
hurting us more than them. Are we prepared to discard valid patches
for this reason?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Thu, Nov 12, 2009 at 9:15 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny
>> <albert(at)nan-tic(dot)com> wrote:
>> > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure:
>> >> Simon Riggs escreveu:
>> >> > So, I
>> >> > propose that we simply ignore patches from developers until they have
>> >> > done sufficient review to be allowed to develop again.
>> >>
>> >> Is it really impolite for a first-contributor, no?
>> >>
>> >
>> > I don't think so, as long as it's properly explained.
>>
>> Personally, I would not propose to impose this rule of first-time
>> contributors, or even second-time contributors.  But by about patch #3
>> I think everyone should be pitching in.
>
> I hate to ask, but how would we enforce this?  Do we no longer apply
> patches for 3rd-time submitters who have not reviewed?  That seems to be
> hurting us more than them.   Are we prepared to discard valid patches
> for this reason?

We just wouldn't assign round-robin reviewers to such patches. If
someone wants to volunteer, more power to them, but we would encourage
people to focus their efforts on the patches of people who were
themselves reviewing. It's important to keep in mind that "valid" is
not a boolean. Some patches are perfect the day they roll in, but not
too many. It takes work to get them committable, and I don't see why
anyone should have an expectation that they can have that help for
themselves without doing the same thing for other people.

All that having been said, the real shortage ATM is of committers
rather than reviewers. We have plenty of them, but many of them
commit almost nothing. I don't want to minimize the contributions of
the non-Tom committers, but Tom is numerically far and away committing
more than anyone else, and not small patches, either. Beyond the
fact that it makes the CommitFest slow, long, and not too much fun for
Tom, it also means that Tom has less time available to do things that
Only Tom Can Do. I venture to say that there will be Great Excitement
about the enhancements to the EPQ machinery and PL/pgsql that Tom has
recently effected. Well, if Tom hadn't had to single-handedly handle
so many patches last CF, maybe he would have done something else cool,
too.

...Robert

Robert Haas wrote:
> >> Personally, I would not propose to impose this rule of first-time
> >> contributors, or even second-time contributors. ?But by about patch #3
> >> I think everyone should be pitching in.
> >
> > I hate to ask, but how would we enforce this? ?Do we no longer apply
> > patches for 3rd-time submitters who have not reviewed? ?That seems to be
> > hurting us more than them. ? Are we prepared to discard valid patches
> > for this reason?
>
> We just wouldn't assign round-robin reviewers to such patches. If
> someone wants to volunteer, more power to them, but we would encourage
> people to focus their efforts on the patches of people who were
> themselves reviewing. It's important to keep in mind that "valid" is
> not a boolean. Some patches are perfect the day they roll in, but not
> too many. It takes work to get them committable, and I don't see why
> anyone should have an expectation that they can have that help for
> themselves without doing the same thing for other people.

OK, but the problem I see there is that the reviewers are there to
assist the committers; if no one reviews something, it just makes more
work for the committers.

> All that having been said, the real shortage ATM is of committers
> rather than reviewers. We have plenty of them, but many of them
> commit almost nothing. I don't want to minimize the contributions of
> the non-Tom committers, but Tom is numerically far and away committing
> more than anyone else, and not small patches, either. Beyond the
> fact that it makes the CommitFest slow, long, and not too much fun for
> Tom, it also means that Tom has less time available to do things that
> Only Tom Can Do. I venture to say that there will be Great Excitement
> about the enhancements to the EPQ machinery and PL/pgsql that Tom has
> recently effected. Well, if Tom hadn't had to single-handedly handle
> so many patches last CF, maybe he would have done something else cool,
> too.

Totally agree.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Thu, Nov 12, 2009 at 11:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> >> Personally, I would not propose to impose this rule of first-time
>> >> contributors, or even second-time contributors. ?But by about patch #3
>> >> I think everyone should be pitching in.
>> >
>> > I hate to ask, but how would we enforce this? ?Do we no longer apply
>> > patches for 3rd-time submitters who have not reviewed? ?That seems to be
>> > hurting us more than them. ? Are we prepared to discard valid patches
>> > for this reason?
>>
>> We just wouldn't assign round-robin reviewers to such patches.  If
>> someone wants to volunteer, more power to them, but we would encourage
>> people to focus their efforts on the patches of people who were
>> themselves reviewing.  It's important to keep in mind that "valid" is
>> not a boolean.  Some patches are perfect the day they roll in, but not
>> too many.  It takes work to get them committable, and I don't see why
>> anyone should have an expectation that they can have that help for
>> themselves without doing the same thing for other people.
>
> OK, but the problem I see there is that the reviewers are there to
> assist the committers;  if no one reviews something, it just makes more
> work for the committers.

That wasn't my intention. I really was assuming that we would just
let those patches drop on the floor, and that they would not be picked
up either by reviewers or committers. I don't think this would cause
as many problems in practice as perhaps you fear, because I think it
will just motivate people to act as reviewers. Writing a patch is
typically more time-consuming than reviewing one, at least IME, with
some exceptions of course. I wouldn't spend 20 hours writing a patch
and then let it fall out because I wasn't willing to spend 2 or 3
hours reviewing someone else's patch, and I don't think other regular
contributors will either.

...Robert

Robert Haas wrote:
> >> We just wouldn't assign round-robin reviewers to such patches. ?If
> >> someone wants to volunteer, more power to them, but we would encourage
> >> people to focus their efforts on the patches of people who were
> >> themselves reviewing. ?It's important to keep in mind that "valid" is
> >> not a boolean. ?Some patches are perfect the day they roll in, but not
> >> too many. ?It takes work to get them committable, and I don't see why
> >> anyone should have an expectation that they can have that help for
> >> themselves without doing the same thing for other people.
> >
> > OK, but the problem I see there is that the reviewers are there to
> > assist the committers; ?if no one reviews something, it just makes more
> > work for the committers.
>
> That wasn't my intention. I really was assuming that we would just
> let those patches drop on the floor, and that they would not be picked
> up either by reviewers or committers. I don't think this would cause
> as many problems in practice as perhaps you fear, because I think it
> will just motivate people to act as reviewers. Writing a patch is
> typically more time-consuming than reviewing one, at least IME, with
> some exceptions of course. I wouldn't spend 20 hours writing a patch
> and then let it fall out because I wasn't willing to spend 2 or 3
> hours reviewing someone else's patch, and I don't think other regular
> contributors will either.

OK, but that is certainly a different system than we have now. In your
system, committers would be told to ignore patches that were submitted
by repeated patch submitters who never review, or even we just never put
on the commit fest page.

I am just trying to nail down exactly how that would work --- that's a
pretty Draconian system.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Thu, Nov 12, 2009 at 11:50 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> >> We just wouldn't assign round-robin reviewers to such patches. ?If
>> >> someone wants to volunteer, more power to them, but we would encourage
>> >> people to focus their efforts on the patches of people who were
>> >> themselves reviewing. ?It's important to keep in mind that "valid" is
>> >> not a boolean. ?Some patches are perfect the day they roll in, but not
>> >> too many. ?It takes work to get them committable, and I don't see why
>> >> anyone should have an expectation that they can have that help for
>> >> themselves without doing the same thing for other people.
>> >
>> > OK, but the problem I see there is that the reviewers are there to
>> > assist the committers; ?if no one reviews something, it just makes more
>> > work for the committers.
>>
>> That wasn't my intention.  I really was assuming that we would just
>> let those patches drop on the floor, and that they would not be picked
>> up either by reviewers or committers.  I don't think this would cause
>> as many problems in practice as perhaps you fear, because I think it
>> will just motivate people to act as reviewers.  Writing a patch is
>> typically more time-consuming than reviewing one, at least IME, with
>> some exceptions of course.  I wouldn't spend 20 hours writing a patch
>> and then let it fall out because I wasn't willing to spend 2 or 3
>> hours reviewing someone else's patch, and I don't think other regular
>> contributors will either.
>
> OK, but that is certainly a different system than we have now.  In your
> system, committers would be told to ignore patches that were submitted
> by repeated patch submitters who never review, or even we just never put
> on the commit fest page.

I think they would probably get added to the CommitFest page and then
marked Rejected with a suitable explanation.

> I am just trying to nail down exactly how that would work --- that's a
> pretty Draconian system.

I don't really agree, but obviously I respect your opinion, and
clearly, this is not a policy that can be implemented without some
degree of consensus. I fear, however, that if we don't motivate
regular contributors to also review, then we will have a shortage of
reviewers, especially highly-qualified reviewers. If there is no
stigma attached to submitting patches and never volunteering to
review, then even people who have reviewed in the past may eventually
decide it isn't worth the effort. I am personally quite tired of
reviewing patches for people who don't in turn review mine (or
someone's). It makes me feel like not working on this project. If we
can solve that problem without implementing a policy of this type,
that is good. I would much prefer to run by the honor system rather
than having to threaten to drop patches, but only if the honor system
actually works.

...Robert

Robert Haas wrote:
>
> That wasn't my intention. I really was assuming that we would just
> let those patches drop on the floor, and that they would not be picked
> up either by reviewers or committers.

Surely it should depend on the nature of the patch.

For an extreme strawman, segfault fixes almost certainly shouldn't
be dropped. Same for docs patches that clarify the product. I
think the majority of my contributions to open source this decade
have been of that nature (a few links to examples in postgres and
postgis follow).

Maybe a better policy would be:
"if you reviewed patches, a reviewer will be assigned -- if
you didn't, your patch is at the mercy of reviewers volunteering
to review it based on their own interest in your patch"
that way patches that the community really wants could get in anyway.

http://postgis.refractions.net/pipermail/postgis-users/2005-April/007762.html
http://archives.postgresql.org/pgsql-performance/2009-03/msg00252.php
http://postgis.refractions.net/pipermail/postgis-users/2005-April/007704.html
http://postgis.refractions.net/pipermail/postgis-devel/2005-April/001341.html

On Fri, Nov 13, 2009 at 2:15 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>> Personally, I would not propose to impose this rule of first-time
>> contributors, or even second-time contributors.  But by about patch #3
>> I think everyone should be pitching in.
>
> I hate to ask, but how would we enforce this?  Do we no longer apply
> patches for 3rd-time submitters who have not reviewed?  That seems to be
> hurting us more than them.   Are we prepared to discard valid patches
> for this reason?

What about people who contribute hours and hours of their time in
other ways? Are they required to contribute even more of their time to
review as well, just to help their own occasional code contributions
get through the process?

(yes, I am thinking largely of me, working tens of hours per week on
Postgres, but perhaps submiting one relatively small patch per
release)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

On Fri, 2009-11-13 at 08:33 +0000, Dave Page wrote:
> On Fri, Nov 13, 2009 at 2:15 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> >> Personally, I would not propose to impose this rule of first-time
> >> contributors, or even second-time contributors. But by about patch #3
> >> I think everyone should be pitching in.
> >
> > I hate to ask, but how would we enforce this? Do we no longer apply
> > patches for 3rd-time submitters who have not reviewed? That seems to be
> > hurting us more than them. Are we prepared to discard valid patches
> > for this reason?
>
> What about people who contribute hours and hours of their time in
> other ways? Are they required to contribute even more of their time to
> review as well, just to help their own occasional code contributions
> get through the process?

Yes, but the rule needs to be hard for a few reasons. If your employer
encourages you to write a patch then they need to be made aware that you
must also do all the other things too: write docs, supply tests, explain
it *and* include review time. Since reviewing your own patch isn't
possible, we must require patch review of other's patches. The rule is
deliberately hard on the contributor to ensure the sponsor understands
the rule and allows more time.

I don't expect it to need to be enforced. If everybody understands it
will be enforced and that there will be some polite reminders, then it
will likely never need to be enforced.

For me, reviewing patches is a great way to learn how to code properly,
learn PostgreSQL and get new ideas.

All the CF manager needs to do is ensure that every patch submitted
chalks up one review. If you think about it, we wouldn't actually need
any rr reviewers at all then, because if we have 20 patches we would
have 20 reviews due. So the whole scheme is self-balancing.

--
Simon Riggs www.2ndQuadrant.com

On Fri, Nov 13, 2009 at 8:50 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> What about people who contribute hours and hours of their time in
>> other ways? Are they required to contribute even more of their time to
>> review as well, just to help their own occasional code contributions
>> get through the process?
>
> Yes, but the rule needs to be hard for a few reasons. If your employer
> encourages you to write a patch then they need to be made aware that you
> must also do all the other things too: write docs, supply tests, explain
> it *and* include review time. Since reviewing your own patch isn't
> possible, we must require patch review of other's patches. The rule is
> deliberately hard on the contributor to ensure the sponsor understands
> the rule and allows more time.

Which is fine for sponsored patches, but my employer doesn't sponsor
me to hack on PostgreSQL directly - we have other staff that are far
more capable of doing that :-). My annual submissions are for my own
personal development/enjoyment/whatever, and are never chosen for my
employer, but because they seem interesting, are common requests from
users, or would help pgAdmin provide improved functionality for users.

The bottom line is, if I'm required to review patches, it will impact
on other community work, which I am almost certainly more qualified to
do and would be more productive at.

That said - in general I don't disagree with the idea of everyone
chipping in, and I do try to do so myself on appropriate patches (for
example, the recent Windows DACL bug fix which I spent a few hours
reviewing and testing - and am still waiting for Magnus to commit
:-p). I just think that *requiring* people to review will ultimately
be counter productive.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

On Fri, 2009-11-13 at 10:26 +0000, Dave Page wrote:
> On Fri, Nov 13, 2009 at 8:50 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> What about people who contribute hours and hours of their time in
> >> other ways? Are they required to contribute even more of their time to
> >> review as well, just to help their own occasional code contributions
> >> get through the process?
> >
> > Yes, but the rule needs to be hard for a few reasons. If your employer
> > encourages you to write a patch then they need to be made aware that you
> > must also do all the other things too: write docs, supply tests, explain
> > it *and* include review time. Since reviewing your own patch isn't
> > possible, we must require patch review of other's patches. The rule is
> > deliberately hard on the contributor to ensure the sponsor understands
> > the rule and allows more time.
>
> Which is fine for sponsored patches, but my employer doesn't sponsor
> me to hack on PostgreSQL directly - we have other staff that are far
> more capable of doing that :-). My annual submissions are for my own
> personal development/enjoyment/whatever, and are never chosen for my
> employer, but because they seem interesting, are common requests from
> users, or would help pgAdmin provide improved functionality for users.
>
> The bottom line is, if I'm required to review patches, it will impact
> on other community work, which I am almost certainly more qualified to
> do and would be more productive at.
>
> That said - in general I don't disagree with the idea of everyone
> chipping in, and I do try to do so myself on appropriate patches (for
> example, the recent Windows DACL bug fix which I spent a few hours
> reviewing and testing - and am still waiting for Magnus to commit
> :-p). I just think that *requiring* people to review will ultimately
> be counter productive.

Requiring people to write docs or any other patch submission rules has
never been counterproductive. People could easily say, "English is not
my first language, therefore I skip all comments and docs". But they
don't, because we require that, as a hard rule. Nobody has ever said
enforcing *those* rules is counter productive. I don't see why adding
new requirements would be a problem - especially since they aim to
address problems with the flow of patches. Change will always seem
strange, but just like commitfests themselves the new way of working has
been quickly adopted without much complaint. Bottom line is if we all
spend all of our time developing and no time reviewing, then we
shouldn't be surprised if there is a review bottleneck. Everybody wants
their patches to go through, and the *fastest* way is actually for
people to assist with review. We just need an easily understood way of
implementing that. The 1:1 suggestion is one way, there may be others.

--
Simon Riggs www.2ndQuadrant.com

On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Requiring people to write docs or any other patch submission rules has
> never been counterproductive. People could easily say, "English is not
> my first language, therefore I skip all comments and docs". But they
> don't, because we require that, as a hard rule. Nobody has ever said
> enforcing *those* rules is counter productive.

Requiring that someone document their own work is very different from
requiring that they spend time reviewing someone elses entirely
unrelated work, possibly in areas of which they have little or no
understanding (which may well be an issue at times).

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Robert Haas wrote:
> I am personally quite tired of
> reviewing patches for people who don't in turn review mine (or
> someone's). It makes me feel like not working on this project. If we
> can solve that problem without implementing a policy of this type,
> that is good. I would much prefer to run by the honor system rather
> than having to threaten to drop patches, but only if the honor system
> actually works.
>
>
>

Organizing contributors on a project like this is like herding cats.
Threats and penalties are unlikely to be effective. This is essentially
a charity where people give in ways that work for them, and you take
whatever they have to give. I'm extremely uncomfortable with the idea of
a prescriptive system. I've proposed them myself in the past, but I have
since come to the realization that it will simply drive people away.

cheers

andrew

Simon Riggs wrote:
> Requiring people to write docs or any other patch submission rules has
> never been counterproductive. People could easily say, "English is not
> my first language, therefore I skip all comments and docs". But they
> don't, because we require that, as a hard rule. Nobody has ever said
> enforcing *those* rules is counter productive. I don't see why adding
> new requirements would be a problem - especially since they aim to
> address problems with the flow of patches. Change will always seem
> strange, but just like commitfests themselves the new way of working has
> been quickly adopted without much complaint. Bottom line is if we all
> spend all of our time developing and no time reviewing, then we
> shouldn't be surprised if there is a review bottleneck. Everybody wants
> their patches to go through, and the *fastest* way is actually for
> people to assist with review. We just need an easily understood way of
> implementing that. The 1:1 suggestion is one way, there may be others.

The docs case is a good example. We do ask people to write docs, but I
don't think we will reject patches if people don't supply docs. I am
not against any of the ideas suggested in this thread --- I am just
pointing out we are heading in a very new direction with the
_requirements_ mentioned.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Fri, 2009-11-13 at 13:34 +0000, Dave Page wrote:
> On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> > Requiring people to write docs or any other patch submission rules has
> > never been counterproductive. People could easily say, "English is not
> > my first language, therefore I skip all comments and docs". But they
> > don't, because we require that, as a hard rule. Nobody has ever said
> > enforcing *those* rules is counter productive.
>
> Requiring that someone document their own work is very different from
> requiring that they spend time reviewing someone elses entirely
> unrelated work, possibly in areas of which they have little or no
> understanding (which may well be an issue at times).

Of course: one requirement is for docs, the other for review.

OTOH they are both additional requirements around submitting a patch.
Once people accept that, it will all work.

All patches require review. If we have no mechanism for providing review
time, then *all* patches will stall. I think it is unfair and unwise to
assume that reviewers just turn up as needed. The reason we are having
this discussion is they plainly don't. We were worried about Tom getting
burnt out by it, now Robert is. I've no problem with arguing against my
specific idea for producing more review time, but if there is no
alternative proposal then all you are saying is "lets not fix the
current problem".

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-11-13 at 08:46 -0500, Andrew Dunstan wrote:
> Organizing contributors on a project like this is like herding cats.
> Threats and penalties are unlikely to be effective.

People have spoken against this because of the enforcement issue. If we
talk about this like we were suggesting hands be removed, then sure,
everybody will be against it. If we enforce it like we do other rules,
such as "if your patch is late, we bump to next commit fest". That is
"enforced in a draconian manner" but nobody thinks that is bad. If you
imagine the suggestion with a softer focus, where we each act in a way
that respects the need for an action that needs to happen, then it will
work.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-11-13 at 08:47 -0500, Bruce Momjian wrote:

> We do ask people to write docs, but I
> don't think we will reject patches if people don't supply docs.

Yes, that is a good example. It's "a rule", plain and simple. Nobody
gets their spleen removed for breaking it, yet it is still somehow
enforced.

I find it strange that suggesting a new rule is opposed on the general
basis that *any* rule cannot be enforced; surely therefore we cannot
have new rules at all, ever? We clearly do have new rules from time to
time. So what's wrong with this new rule?

Should we update the FAQ to say, "enclosing docs with a patch is a rule,
but actually its not really and you only suffer mild rebuke if you break
it and can therefore be ignored"?

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> On Fri, 2009-11-13 at 08:47 -0500, Bruce Momjian wrote:
>
> > We do ask people to write docs, but I
> > don't think we will reject patches if people don't supply docs.
>
> Yes, that is a good example. It's "a rule", plain and simple. Nobody
> gets their spleen removed for breaking it, yet it is still somehow
> enforced.
>
> I find it strange that suggesting a new rule is opposed on the general
> basis that *any* rule cannot be enforced; surely therefore we cannot
> have new rules at all, ever? We clearly do have new rules from time to
> time. So what's wrong with this new rule?
>
> Should we update the FAQ to say, "enclosing docs with a patch is a rule,
> but actually its not really and you only suffer mild rebuke if you break
> it and can therefore be ignored"?

Well, right now we ask for docs, but if they are not supplied, I think
we just write them ourselves. Is a different enforcement method being
suggested here?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Fri, 2009-11-13 at 09:31 -0500, Bruce Momjian wrote:

> Well, right now we ask for docs, but if they are not supplied, I think
> we just write them ourselves. Is a different enforcement method being
> suggested here?

And we never bump late patches, nor reject them if sent in missing
format etc.

We enforce all manner of rules. You tell me this is your main function
on the project even. Is there now no enforcement of any rule, just
because I propose a new one?

Address the main question: how will we get more review time? There are
many other possible proposals, so please lets hear them. (And how would
they be enforced?)

--
Simon Riggs www.2ndQuadrant.com

On Fri, Nov 13, 2009 at 8:47 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> The docs case is a good example.  We do ask people to write docs, but I
> don't think we will reject patches if people don't supply docs.  I am
> not against any of the ideas suggested in this thread --- I am just
> pointing out we are heading in a very new direction with the
> _requirements_ mentioned.

We reject patches for lack of docs all the time. We certainly don't
have a policy that the reviewer or committer will write the docs for
you if you fail to write them yourself. Sometimes the reviewer or
committer will help copy edit, or will revise, but in most cases they
won't write them from scratch.

Of course, we don't reject such patches PERMANENTLY - people just add
the docs and resubmit.

...Robert

Robert Haas wrote:
> On Fri, Nov 13, 2009 at 8:47 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
>> The docs case is a good example. We do ask people to write docs, but I
>> don't think we will reject patches if people don't supply docs. I am
>> not against any of the ideas suggested in this thread --- I am just
>> pointing out we are heading in a very new direction with the
>> _requirements_ mentioned.
>>
>
> We reject patches for lack of docs all the time. We certainly don't
> have a policy that the reviewer or committer will write the docs for
> you if you fail to write them yourself. Sometimes the reviewer or
> committer will help copy edit, or will revise, but in most cases they
> won't write them from scratch.
>
> Of course, we don't reject such patches PERMANENTLY - people just add
> the docs and resubmit.
>
>
>

In that case people are working on their own patches. That's quite
different from asking/requiring them to work on somebody else's.

cheers

andrew

On Fri, Nov 13, 2009 at 8:46 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Robert Haas wrote:
>>
>> I am personally quite tired of
>> reviewing patches for people who don't in turn review mine (or
>> someone's).  It makes me feel like not working on this project.  If we
>> can solve that problem without implementing a policy of this type,
>> that is good.  I would much prefer to run by the honor system rather
>> than having to threaten to drop patches, but only if the honor system
>> actually works.
>
> Organizing contributors on a project like this is like herding cats. Threats
> and penalties are unlikely to be effective. This is essentially a charity
> where people give in ways that work for them, and you take whatever they
> have to give. I'm extremely uncomfortable with the idea of a prescriptive
> system. I've proposed them myself in the past, but I have since come to the
> realization that it will simply drive people away.

I think you're entirely missing the point. If you want to spend large
amounts of your time reviewing patches from people who won't in turn
review yours, that is *absolutely wonderful*. It would be a huge help
to the project, especially because you have the ability not only to
review them, but also commit them, an area in which we are hugely
strapped for qualified people who have the ability to commit
substantial time to the project. I would be more than happy to cut
back on managing CommitFests and reviewing patches and focus on
writing my own patches and letting you fix them up and commit them. I
have several good, interesting ideas for really cool patches that I
don't have time to write, and they are posted on the developer Wiki if
you'd like to go read them.

Personally, I believe that I take more of an interest in other
people's patches than the average contributor. I am interested in
reviewing them, and I would be willing to put even more work in
exchange for a commit bit, but that offer has not yet been
forthcoming; maybe some day. I have done a large percentage of the
management work for both of the last two CommitFests, during which
time I have also reviewed 8 patches. That work was long, hard,
difficult, and time-consuming. I wish to get something back for it,
and specifically what I want to get back for it is the willingness of
other contributors to review my patches when and if I can get enough
of a break from running CommitFests and reviewing patches to write
them. I don't find that setting that expectation is either
unreasonable or unfair, and I'm sorry that you and Bruce apparently
feel otherwise.

What I would like to know is - assuming you don't want to do it
yourself, and you don't want to require other *regular contributors*
to do it - then who is going to review MY patches? Keep in mind that
this is a problem that *does not apply to you*. You are a committer.
If no one reviews your patch, you will eventually go ahead and commit
it anyway. If no one reviews my patch, it doesn't go in. In fact,
even if someone DOES review it, it doesn't necessarily go in, but at
least the odds are better. Please don't sabotage my effort to ensure
an adequate supply of reviewers unless you have a competing proposal.

...Robert

* Andrew Dunstan <andrew(at)dunslane(dot)net> [091113 09:52]:

> In that case people are working on their own patches. That's quite
> different from asking/requiring them to work on somebody else's.

But is it?

Just s/patches/itches/

i.e. The "patched code implenting feature $X" is their main itch... They
scratch that, and massage some other places (like docs and tests) to make
their itch more palatable/pleasant for the rest of the community,
because the community expects docs, tests, and reviews.

a.

--
Aidan Van Dyk Create like a god,
aidan(at)highrise(dot)ca command like a king,
http://www.highrise.ca/ work like a slave.

On Fri, Nov 13, 2009 at 8:34 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
>> Requiring people to write docs or any other patch submission rules has
>> never been counterproductive. People could easily say, "English is not
>> my first language, therefore I skip all comments and docs". But they
>> don't, because we require that, as a hard rule. Nobody has ever said
>> enforcing *those* rules is counter productive.
>
> Requiring that someone document their own work is very different from
> requiring that they spend time reviewing someone elses entirely
> unrelated work, possibly in areas of which they have little or no
> understanding (which may well be an issue at times).

I think this is overstating the level of contribution that is being
asked, at least by me. Every CommitFest is full of a bunch of little
patches that make small changes and need to be reviewed. Anyone who
is reasonably familiar with C (and most of our regular contributors
are) can take a little time to understand what one of those patches is
doing and check it for style and functionality. I can usually review
one of those patches in about 2 hours knowing nothing about that area
of the system.

What I object to about the present system is that the last two
CommitFests have gone like this, for me:

1. First, I assign reviewers to as many patches as we have reviewers for.
2. Then, I ask the reviewers who haven't done so to actually complete
the reviews.
3. Then, I ask the patch submitters who haven't done so to update
their patches, and/or bounce the patches for lack of an update.
4. Then, I ask the reviewers who don't do it without prompting to
check over the revised patches.
...at this point we are about 2 weeks into the CommitFest...
5. Then, I try to find reviewers for the patches that haven't been
reviewed yet and start steps 1-4 over with the remaining patches.
6. Concurrently, I review several patches myself.

Step #5 is the one that is really irritating to me. If there are 45
patches in the CommitFest and 15 to 20 people who didn't submit
anything sign up to review and the committers take 7 or 8 patches
directly, why is the number of reviewers still 10 or 15 less than the
number of patches? My diagnosis is that the people who submit 2 or 3
or 4 or 5 patches to the CommitFest think to themselves "well, if I
sign up to review, I might not have time to update all of these
patches and get them committed during this CommitFest, so I'll let
someone else do it". As far as I can see, this is the exact opposite
of how the process is supposed to work: during the CommitFest, people
are supposed to stop working on their own patches and review patches
belonging to other people. As it is, the people who are actually
willing to review are getting asked to review multiple patches so that
other people can review none at all. Some people are willing to do
that, and that is fine, but many are not, and even for the ones who
are, after a certain point, it seems unfair to ask it.

To put this another way, if everyone who submitted a patch reviewed a
patch, we could finish up each CommitFest in 2-3 weeks instead of a
whole month, except that the committing would drag out for the rest of
the month unless someone other than Tom is willing to help to a
greater degree than in the most recent CommitFest. That would
substantially increase the time available for everyone to work on
their own patches.

...Robert

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> All the CF manager needs to do is ensure that every patch submitted
> chalks up one review. If you think about it, we wouldn't actually need
> any rr reviewers at all then, because if we have 20 patches we would
> have 20 reviews due. So the whole scheme is self-balancing.

Well, no, that's *far* too optimistic/simplistic, because it imagines
that every review is worth the same. What we lack is not just review
time but qualified review time, ie, comments from someone who's already
familiar with the portion of the code base that's being patched.

Now when the current reviewing process was proposed, there were two
separate goals in mind. One was to take whatever incremental load
we could off the eventual committer's work, by catching obvious
mistakes, making sure the docs were up to snuff, etc. The other was
the idea that reviewing would in itself improve the skills of our
development community, by making people read code that they wouldn't
have read otherwise, and that eventually we'd have more committer-grade
people just because of all the reviewing they'd done. (The jury is
still out on whether that will work, but in any case it's a long-term
project.)

The problem at the moment seems to not be lack of first-level review
time but lack of qualified review. I don't know what we do about that.
Requiring people who have submitted one or two patches to do reviews
isn't going to produce more of the latter, it's going to produce more
of the former. Especially if the patches available to be reviewed
are working in areas they haven't looked at before.

regards, tom lane

On Fri, Nov 13, 2009 at 10:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> All the CF manager needs to do is ensure that every patch submitted
>> chalks up one review. If you think about it, we wouldn't actually need
>> any rr reviewers at all then, because if we have 20 patches we would
>> have 20 reviews due. So the whole scheme is self-balancing.
>
> Well, no, that's *far* too optimistic/simplistic, because it imagines
> that every review is worth the same.  What we lack is not just review
> time but qualified review time, ie, comments from someone who's already
> familiar with the portion of the code base that's being patched.

Right, but I think we're more likely to find such people among the
pool of existing contributors than we are among people who don't write
patches themselves but happen to volunteer to review.

I think Simon's idea of requiring 1 review per patch probably IS a bit
overly simplistic - for one thing, someone who submits 10 patches, as
I did in the July CommitFest, can scarcely be expected to also review
10 patches. (Even if they were willing, it would make the CommitFest
longer, not shorter.) But I don't think they should get by reviewing
none, either, especially if they're submitting patches to every
CommitFest.

It's not my idea that we should punish someone like Dave Page who does
a lot of PostgreSQL work and occasionally writes a patch. What I'm
complaining about is people who submit patches regularly and rarely or
never review. We have enough volunteers to cover new and occasional
patch submitters; sometimes those reviews are not quite as thorough,
but new and occasional contributors tend to submit relatively simple
patches anyway, so it's not a catastrophe. It's the regular patch
submitters who, IMHO, most need to be involved.

...Robert

Robert Haas wrote:
> On Fri, Nov 13, 2009 at 10:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>>> All the CF manager needs to do is ensure that every patch submitted
>>> chalks up one review. If you think about it, we wouldn't actually need
>>> any rr reviewers at all then, because if we have 20 patches we would
>>> have 20 reviews due. So the whole scheme is self-balancing.
>> Well, no, that's *far* too optimistic/simplistic, because it imagines
>> that every review is worth the same. What we lack is not just review
>> time but qualified review time, ie, comments from someone who's already
>> familiar with the portion of the code base that's being patched.
>
> Right, but I think we're more likely to find such people among the
> pool of existing contributors than we are among people who don't write
> patches themselves but happen to volunteer to review.
>
> I think Simon's idea of requiring 1 review per patch probably IS a bit
> overly simplistic - for one thing, someone who submits 10 patches, as
> I did in the July CommitFest, can scarcely be expected to also review
> 10 patches. (Even if they were willing, it would make the CommitFest
> longer, not shorter.) But I don't think they should get by reviewing
> none, either, especially if they're submitting patches to every
> CommitFest.
>
> It's not my idea that we should punish someone like Dave Page who does
> a lot of PostgreSQL work and occasionally writes a patch. What I'm
> complaining about is people who submit patches regularly and rarely or
> never review. We have enough volunteers to cover new and occasional
> patch submitters; sometimes those reviews are not quite as thorough,
> but new and occasional contributors tend to submit relatively simple
> patches anyway, so it's not a catastrophe. It's the regular patch
> submitters who, IMHO, most need to be involved.

I think we (the commitfest manager?) should simply send polite message
to any regulars who submits patches but hasn't volunteered for review.
Along the lines of:

"Hi XXX. You've submitted a patch to the commitfest. As you know, this
is a community process and the it depends on volunteers like you. It
would be extremely helpful if you could pick a patch that interests you
from the list at commitfest.postgresql.org, mark yourself as a reviewer,
and review and/or test it as thoroughly as you can. Remember that the
faster other patches are reviewed, the faster others get to review your
patch and the faster the commitfest can be closed and the version can be
released."

The commitfest manager can apply common sense: if there's plenty of
reviewers already and not many patches, there's no need to reach out to
more people. OTOH, if there's a shortage, he can e.g. go through old
commitfests too and beg people who have contributed in the past.

Think of a fundraiser who calls around people, begging for donations.
This is the same, except that we're begging for people's time instead of
money. Or think of donating blood. If there's a shortage of blood of a
certain type, they will send emails to past donors, asking to come donate.

I agree with Tom though that we don't really need a huge pool of people
who chip in with one hour per month. We need people who know the
codebase pretty well, and who can spend a fair amount of time to do
thorough review of complex patches. There is a few dozen or so people
out there who have enough knowledge on various parts of the system, or
are good at writing documentation, or good at testing. The question is
how to get them to donate more time. I'm proposing that we ask them to.
It can't hurt.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Fri, 2009-11-13 at 09:31 -0500, Bruce Momjian wrote:
>
> > Well, right now we ask for docs, but if they are not supplied, I think
> > we just write them ourselves. Is a different enforcement method being
> > suggested here?
>
> And we never bump late patches, nor reject them if sent in missing
> format etc.
>
> We enforce all manner of rules. You tell me this is your main function
> on the project even. Is there now no enforcement of any rule, just
> because I propose a new one?

Enforcing civil behavior in the group, in a private way, is different
from having a public policy that requires patch review.

Again, I am not against this change --- I am just pointing out it is new
territory for us.

> Address the main question: how will we get more review time? There are
> many other possible proposals, so please lets hear them. (And how would
> they be enforced?)

Not sure, but I would like to point out the bottleneck is not currently
the reviewers but the ability to give final approval and commit it. And
again, the commit process is very fast --- it is that final approval
that is hard.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

2009/11/14 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> I think we (the commitfest manager?) should simply send polite message
> to any regulars who submits patches but hasn't volunteered for review.
> Along the lines of:
>

I certainly endorse Heikki's suggestion, but I wonder if we can do
more to make reviewing patches an attractive option.

As Tom notes, reviewing somebody else's patch just isn't as much fun
as working on your own. Robert notes that reviewing patches is a
great way to learn about the codebase, and I concur. But in terms of
hacker satisfaction it just doesn't compare to creating something, and
I think the ratios of submitters to reviewers in recent CFs vindicate
me here.

I'm thinking of something like a Reviewer Hall of Fame, or Honour
Roll. During and after a commitfest, it shows how many reviews have
been completed by each person [1].

This could be included in the Weekly News at the CF's conclusion.

One of the things that people get out of contributing to an OSS
project is the recognition of their peers. Well then, let's leverage
that by acclaiming the people who put in a lot of effort reviewing,
loudly and publicly. The louder and more public, the more powerful
the incentive.

Cheers,
BJ

[1] perhaps with some subjective weighting with respect to patch
complexity / depth of review.

Robert Haas wrote:
> Please don't sabotage my effort to ensure
> an adequate supply of reviewers unless you have a competing proposal.
>
>

I don't think you can reasonably demand this. If I don't think your
suggestion is going to improve matters I have a right to say so.

cheers

andrew

On Fri, Nov 13, 2009 at 12:18 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I agree with Tom though that we don't really need a huge pool of people
> who chip in with one hour per month. We need people who know the
> codebase pretty well, and who can spend a fair amount of time to do
> thorough review of complex patches. There is a few dozen or so people
> out there who have enough knowledge on various parts of the system, or
> are good at writing documentation, or good at testing. The question is
> how to get them to donate more time. I'm proposing that we ask them to.
> It can't hurt.

I agree.

...Robert

On Fri, Nov 13, 2009 at 12:32 PM, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> I'm thinking of something like a Reviewer Hall of Fame, or Honour
> Roll.  During and after a commitfest, it shows how many reviews have
> been completed by each person [1].
>
> This could be included in the Weekly News at the CF's conclusion.
>
> One of the things that people get out of contributing to an OSS
> project is the recognition of their peers.  Well then, let's leverage
> that by acclaiming the people who put in a lot of effort reviewing,
> loudly and publicly.  The louder and more public, the more powerful
> the incentive.

I think this would be a nice thing to do. +1 for considering
complexity of patches in making this determination.

...Robert

Simon Riggs wrote:
> All the CF manager needs to do is ensure that every patch submitted
> chalks up one review. If you think about it, we wouldn't actually need
> any rr reviewers at all then, because if we have 20 patches we would
> have 20 reviews due. So the whole scheme is self-balancing
In fact, just suggesting the guideline that everyone who submits a patch
should review one here was sufficient to pull in a number of submitters
who volunteered to do a single review as well, moving some distance
toward what you're describing. It seems we had a perception here that
joining rrreviewers "subscribed" you to doing multiple patch reviews;
I've let multiple submitters who were trying to help out know it's OK to
just grab one patch and review without even getting involved on that list.

Take a look at
https://commitfest.postgresql.org/action/commitfest_view?id=4 right
now. I've been suggesting to people that they assign themselves to the
patches they like, and it's nearing completely populated two days before
the CommitFest has even started. I have 6 reviewers that haven't been
assigned anything yet and there are only 8 unassigned patches out
there. In several cases, assigning the reviewer turned out to be quite
easy because so many submitters joined in--just assign someone who
submitted a patch in the same area.

So it far it looks sufficient to introduce the expectation that
submitters should also do a review, without even needing to make that a
firm rule. That helps increase the reviewer pool significantly,
addressing the general problem Robert has been fighting, while not
forcing people like Dave who have other pulls on their time into a
review role. We'll see whether the follow-through here is good or not,
maybe this will decay yet. For now, simply telling submitters that the
review of their own patches might be influenced by whether they do a
good job reviewing someone else's has improved things considerably over
past CommitFests, and it's hard to imagine how someone could complain
about a guideline that fair.

The most difficult part here remains finding reviewers for the really
big patches.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Fri, Nov 13, 2009 at 12:37 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Robert Haas wrote:
>> Please don't sabotage my effort to ensure
>> an adequate supply of reviewers unless you have a competing proposal.
>
> I don't think you can reasonably demand this. If I don't think your
> suggestion is going to improve matters I have a right to say so.

I've never disputed the right of you or anyone else to say whatever
they like. Just to be clear, I don't think that mandating reviews is
the best idea anyone has ever had, and I don't rule out the
possibility that in solving one problem it might create some others.
I think those problems are likely solvable, but I might be wrong, and
in any event, it's clearly better for it to be a voluntary system.

As far as I can tell, the major objection to having it be mandatory is
that it might drive some people away. My major argument for why that
isn't the case is that the mere fact that we are even *discussing*
whether it should be mandatory has led to a bumper crop of reviewers,
including several of the people who fall into the category I've been
discussing. So maybe we don't need to make it mandatory: maybe we
just need to discuss making it mandatory every 6 months or so. :-)

Anyhow, as Bruce pointed out on another message, in some sense we are
getting sidetracked. Good reviewers opting out of the system *is* a
problem, but lack of a sufficient number of sufficiently involved
committers is a bigger one.

...Robert

On Fri, November 13, 2009 1:04 pm, Robert Haas wrote:
> the mere fact that we are even *discussing*
> whether it should be mandatory has led to a bumper crop of reviewers,

Non sequitur.

I think it is more likely that the "bumper crop of reviewers" is due
to the lengthy discussion about the lack of reviewers.
--
nathan wagner

On Fri, 2009-11-13 at 10:12 -0500, Robert Haas wrote:
> Keep in mind that
> this is a problem that *does not apply to you*. You are a committer.
> If no one reviews your patch, you will eventually go ahead and commit
> it anyway. If no one reviews my patch, it doesn't go in.

That is the problem.

I understand that committers may not care, or agree. But blindness to a
problem doesn't mean it doesn't exist.

Making anyone a committer doesn't change that problem, it just changes
*who* experiences the problem. But my view is that if *any* developer
experiences a problem then *we* the community experience a problem.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-11-13 at 10:55 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > All the CF manager needs to do is ensure that every patch submitted
> > chalks up one review. If you think about it, we wouldn't actually need
> > any rr reviewers at all then, because if we have 20 patches we would
> > have 20 reviews due. So the whole scheme is self-balancing.
>
> Well, no, that's *far* too optimistic/simplistic, because it imagines
> that every review is worth the same. What we lack is not just review
> time but qualified review time, ie, comments from someone who's already
> familiar with the portion of the code base that's being patched.

I agree your point, but the principle is clear. Give back what you have
received. If somebody has submitted a complex patch then we expect them
to take a complex review. Once the principle has been established, we
will all follow it...and my point is...sponsors will then also be forced
to follow this also.

Dave may worry I am discussing his company. Actually, I'm not. I'm more
worried about my sponsors. If I am forced to do review, then I will have
to do it. If it is optional, then my sponsors will not pay for it. End
of story. Then we end up with a patch that is only reviewed if others
agree to do so. Sponsors don't win, but they can't see why.

I don't believe that you will personally fill the gaps in our review
process for ever and ever: that *would* be optimism. We need a system
that works in the longer term.

> Now when the current reviewing process was proposed, there were two
> separate goals in mind. One was to take whatever incremental load
> we could off the eventual committer's work, by catching obvious
> mistakes, making sure the docs were up to snuff, etc. The other was
> the idea that reviewing would in itself improve the skills of our
> development community, by making people read code that they wouldn't
> have read otherwise, and that eventually we'd have more committer-grade
> people just because of all the reviewing they'd done. (The jury is
> still out on whether that will work, but in any case it's a long-term
> project.)
>
> The problem at the moment seems to not be lack of first-level review
> time but lack of qualified review. I don't know what we do about that.
> Requiring people who have submitted one or two patches to do reviews
> isn't going to produce more of the latter, it's going to produce more
> of the former. Especially if the patches available to be reviewed
> are working in areas they haven't looked at before.

Review more, and we get better at it. Practice is the only way I know to
get better at something.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-11-13 at 10:46 -0500, Robert Haas wrote:

> To put this another way, if everyone who submitted a patch reviewed a
> patch, we could finish up each CommitFest in 2-3 weeks instead of a
> whole month

Agreed. That's the idea, lets go with it to see if it works.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-11-13 at 12:56 -0500, Greg Smith wrote:

> For now, simply telling submitters that the
> review of their own patches might be influenced by whether they do a
> good job reviewing someone else's has improved things considerably
> over past CommitFests, and it's hard to imagine how someone could
> complain about a guideline that fair.

OK, I guess if we're already doing it...

--
Simon Riggs www.2ndQuadrant.com

On Friday 13 November 2009 18:56:01 Greg Smith wrote:
> Take a look at
> https://commitfest.postgresql.org/action/commitfest_view?id=4 right
> now. I've been suggesting to people that they assign themselves to the
> patches they like, and it's nearing completely populated two days before
> the CommitFest has even started. I have 6 reviewers that haven't been
> assigned anything yet and there are only 8 unassigned patches out
> there. In several cases, assigning the reviewer turned out to be quite
> easy because so many submitters joined in--just assign someone who
> submitted a patch in the same area.
I have to admit that at least for me personally its way much easer to get
started on a patch one finds interesting than when not - especially if the
reviewing time slot is after a 10-12h workday or such. To the point where
starting to review a personally "boring" patch takes more time than the review
would have taken itself.

Embarassed,

Andres

On Fri, 2009-11-13 at 23:10 +0100, Andres Freund wrote:
> I have to admit that at least for me personally its way much easer to get
> started on a patch one finds interesting than when not

I find it much easier to get interested in a patch after I get started
reviewing it ;)

Seriously, that's happened with at least a couple patches that have been
assigned to me. Either the code itself is interesting, or the feature is
useful enough that I would want to learn about it anyway.

Regards,
Jeff Davis

Robert Haas wrote:
> Anyhow, as Bruce pointed out on another message, in some sense we are
> getting sidetracked. Good reviewers opting out of the system *is* a
> problem, but lack of a sufficient number of sufficiently involved
> committers is a bigger one.

I want to thank everyone for the fine discussion we are having about the
community patch review and commit process. I am somewhat relieved that
others share the concerns I expressed earlier.

Having read the discussion and heard people's opinions, I am now
thinking that I need to get more involved in committing patches. I used
to do a lot of that, but backed away from it when the commit fest
process started so the new system could develop on its own, without
being swayed by my involvement. It also coincided with the start of a
heavy travel period for me and involvement in side projects, like
pg_migrator.

I am probably more able than most to adjust my schedule to devote time
to committing things. I am not great at committing patches, but with
the help of others, I think I can help carry the load.

Ironically, I am leaving tomorrow for a trip to Japan and China, and
return on November 26 (Thanksgiving in the USA), so the bad news is I
can't help much until I return. The good news is that I don't have any
trips scheduled after that, and trips are usually scheduled months in
advance, so it is unlikely I will be traveling much in the coming
months.

I am thinking I need to travel less and focus on helping commit patches.
We now have a very capable world-wide group of Postgres community
speakers so my decline from the conference circuit should have little
impact.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Fri, Nov 13, 2009 at 11:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Having read the discussion and heard people's opinions, I am now
> thinking that I need to get more involved in committing patches.

Woot.

...Robert

Bruce Momjian wrote:
> I am probably more able than most to adjust my schedule to devote time
> to committing things.
>

Yes, time is what has restricted what I can do. I'll try to do a bit
more for this coming commitfest, but I'm rather sad that I haven't made
a more substantial contribution to this release.

cheers

andrew

andrew(at)dunslane(dot)net (Andrew Dunstan) writes:
> Robert Haas wrote:
>> I am personally quite tired of reviewing patches for people who don't
>> in turn review mine (or someone's). It makes me feel like not
>> working on this project. If we can solve that problem without
>> implementing a policy of this type, that is good. I would much
>> prefer to run by the honor system rather than having to threaten to
>> drop patches, but only if the honor system actually works.
>
> Organizing contributors on a project like this is like herding
> cats. Threats and penalties are unlikely to be effective. This is
> essentially a charity where people give in ways that work for them,
> and you take whatever they have to give. I'm extremely uncomfortable
> with the idea of a prescriptive system. I've proposed them myself in
> the past, but I have since come to the realization that it will simply
> drive people away.

Ah, but the thing is, what was proposed wasn't "totally evilly
draconian."

There's a difference between:

"You haven't reviewed any patches - we'll ignore you forever!"

and

"Since you haven't reviewed any patches, we are compelled to defer your
patches until the next CommitFest."

It's enough pain to make people think, but it's not *totally* punitive.
--
"I really only meant to point out how nice InterOp was for someone who
doesn't have the weight of the Pentagon behind him. I really don't
imagine that the Air Force will ever be able to operate like a small,
competitive enterprise like GM or IBM." -- Kent England

On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:

> Ah, but the thing is, what was proposed wasn't "totally evilly
> draconian."
>
> There's a difference between:
>
> "You haven't reviewed any patches - we'll ignore you forever!"
>
> and
>
> "Since you haven't reviewed any patches, we are compelled to defer your
> patches until the next CommitFest."
>
> It's enough pain to make people think, but it's not *totally* punitive.

It is important to remember we are all volunteers here. Any increase to
the barrier of contribution is a bad one.

Joshua D. Drake

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander

jd(at)commandprompt(dot)com ("Joshua D. Drake") writes:
> On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
>
>> Ah, but the thing is, what was proposed wasn't "totally evilly
>> draconian."
>>
>> There's a difference between:
>>
>> "You haven't reviewed any patches - we'll ignore you forever!"
>>
>> and
>>
>> "Since you haven't reviewed any patches, we are compelled to defer
>> your patches until the next CommitFest."
>>
>> It's enough pain to make people think, but it's not *totally*
>> punitive.
>
> It is important to remember we are all volunteers here. Any increase to
> the barrier of contribution is a bad one.

But this *isn't* a barrier to contribution, at least not notably more
than the already existant issue that a paucity of reviewers is a barrier
to contribution.

It represents a policy for triaging review efforts with a bias in favor
of those that *are* contributing to the reviewers' list.

I don't think it's unjust for those that contribute to the review
process to get more favorable scheduling of reviews to their patches.

If we get so many reviewers that such triaging becomes unnecessary, then
it may automatically *not* be a problem.
--
(format nil "~S(at)~S" "cbbrowne" "acm.org")
http://linuxfinances.info/info/slony.html
"Bother," said Pooh, as he deleted his root directory.

On Mon, Nov 16, 2009 at 12:17 PM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
>
>> Ah, but the thing is, what was proposed wasn't "totally evilly
>> draconian."
>>
>> There's a difference between:
>>
>>  "You haven't reviewed any patches - we'll ignore you forever!"
>>
>> and
>>
>>  "Since you haven't reviewed any patches, we are compelled to defer your
>>   patches until the next CommitFest."
>>
>> It's enough pain to make people think, but it's not *totally* punitive.
>
> It is important to remember we are all volunteers here. Any increase to
> the barrier of contribution is a bad one.

True. But "not enough reviewers to review all the patches we get" is
also a barrier to contribution.

...Robert

On Mon, 2009-11-16 at 12:42 -0500, Robert Haas wrote:
> On Mon, Nov 16, 2009 at 12:17 PM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
> >
> >> Ah, but the thing is, what was proposed wasn't "totally evilly
> >> draconian."
> >>
> >> There's a difference between:
> >>
> >> "You haven't reviewed any patches - we'll ignore you forever!"
> >>
> >> and
> >>
> >> "Since you haven't reviewed any patches, we are compelled to defer your
> >> patches until the next CommitFest."
> >>
> >> It's enough pain to make people think, but it's not *totally* punitive.
> >
> > It is important to remember we are all volunteers here. Any increase to
> > the barrier of contribution is a bad one.
>
> True. But "not enough reviewers to review all the patches we get" is
> also a barrier to contribution.

No. It is a barrier of contribution not to contribution.

The types of current structure that are being considered are punitive
regardless of the softness of wording.

This is certainly not an easy problem to solve and I am not saying I
have a better solution (although something more personal and direct such
as the way Selena helps user groups seems more appropriate).

Sincerely,

Joshua D. Drake

>
> ...Robert
>

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander

On Mon, Nov 16, 2009 at 1:08 PM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
>> True.  But "not enough reviewers to review all the patches we get" is
>> also a barrier to contribution.
>
> No. It is a barrier of contribution not to contribution.

I am not sure exactly what that means, but I agree that it isn't quite
the same. Backing up a minute, AIUI, the CommitFest process was
created to solve the problem that patches weren't getting reviewed in
a timely fashion. To address that problem, dedicated times were
created in each release cycle for people to stop working on their own
patches and review patches from other contributors. I haven't been
around long enough to be able to compare from personal experience, but
I think generally what I've heard is that the new process is a big
improvement. But, there are some problems, and speaking from
experience, one of those problems is that reviewing patches and
running CommitFests is long, hard, and difficult when not enough
people volunteer to review, or not enough committers volunteer to
commit.

I guess I agree with your statement that the structures that are being
proposed are punitive, although perhaps I might choose the word
coercive instead. Clearly, the preferable solution is for people to
volunteer. But if they don't, we haven't got a lot of options.
Perhaps by encouraging them to volunteer and recognizing their
contributions when they do volunteer, we can get the number of
volunteers back up to an adequate level. If after doing those things
we still don't have enough volunteers, we're not going to be able to
review all the patches. Should that occur, we'll have to decide which
ones to review and which ones to skip. Maybe we'll just let people
volunteer and any patches for which nobody volunteers will fall on the
floor or be forever postponed to the next CommitFest. Maybe we'll try
to assign reviewers preferentially to first-time contributors and
those who are themselves reviewing, as I'm suggesting. Or maybe we'll
handle it some other way. I don't know. It seems we don't have to
decide yet.

...Robert

On Mon, Nov 16, 2009 at 12:41:02PM -0500, Chris Browne wrote:
> jd(at)commandprompt(dot)com ("Joshua D. Drake") writes:
> > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
> >
> >> Ah, but the thing is, what was proposed wasn't "totally evilly
> >> draconian."
> >>
> >> There's a difference between:
> >>
> >> "You haven't reviewed any patches - we'll ignore you forever!"
> >>
> >> and
> >>
> >> "Since you haven't reviewed any patches, we are compelled to
> >> defer your patches until the next CommitFest."
> >>
> >> It's enough pain to make people think, but it's not *totally*
> >> punitive.
> >
> > It is important to remember we are all volunteers here. Any
> > increase to the barrier of contribution is a bad one.
>
> But this *isn't* a barrier to contribution, at least not notably
> more than the already existant issue that a paucity of reviewers is
> a barrier to contribution.
>
> It represents a policy for triaging review efforts with a bias in
> favor of those that *are* contributing to the reviewers' list.
>
> I don't think it's unjust for those that contribute to the review
> process to get more favorable scheduling of reviews to their
> patches.
>
> If we get so many reviewers that such triaging becomes unnecessary,
> then it may automatically *not* be a problem.

In the PostgreSQL Weekly News, I track patches, and apparently at
least one person reads that section. Would it be helpful to track
reviews somehow during commitfests with the reviewers' names
prominently attached?

It's a more positive approach, and like many others, I really prefer
those types of approaches, even if I grump occasionally. :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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

2009/11/17 David Fetter <david(at)fetter(dot)org>:
> In the PostgreSQL Weekly News, I track patches, and apparently at
> least one person reads that section.  Would it be helpful to track
> reviews somehow during commitfests with the reviewers' names
> prominently attached?
>

Yes. See also my suggestion [1] that we do a Reviewer "Honour Roll"
or "Hall of Fame" at the end of the CF, also published in the PWN.

One of the rewards for getting a patch into the tree is having your
name immortalised in the commit log. There's no such compensation for
reviewing patches.

I think creating incentives to review is going to be more potent and
more enjoyable for everyone involved than punitive measures.

Cheers,
BJ

[1] http://archives.postgresql.org/message-id/37ed240d0911130932i3b48849csb8cbae061abf11e4@mail.gmail.com

Brendan Jurd <direvus(at)gmail(dot)com> writes:
> One of the rewards for getting a patch into the tree is having your
> name immortalised in the commit log. There's no such compensation for
> reviewing patches.

Well, that could be fixed: instead of

blah blah blah

Joe Coder

we could write

blah blah blah

Joe Coder, reviewed by X and Y

Although keeping track of just who to credit might be a bit tricky.
I'd be happy to commit to crediting whoever is listed in the CF entry
for the patch, but sometimes other people have chimed in as much or
more as the nominal reviewer.

regards, tom lane

Brendan Jurd wrote:
> One of the rewards for getting a patch into the tree is having your
> name immortalised in the commit log. There's no such compensation for
> reviewing patches.
>
> I think creating incentives to review is going to be more potent and
> more enjoyable for everyone involved than punitive measures.
>
>
>

Indeed. I once suggested only half jokingly that we should have a "Coder
of the month" award. Maybe a "Reviewer of the month" award would also be
good. Seriously, the major benefit most people get from contributing
(apart from good karma and a warm inner glow) is kudos, and we should
possibly lay it on a bit thicker.

cheers

andrew

On Mon, 2009-11-16 at 19:15 -0500, Andrew Dunstan wrote:
>
> Brendan Jurd wrote:
> > One of the rewards for getting a patch into the tree is having your
> > name immortalised in the commit log. There's no such compensation for
> > reviewing patches.
> >
> > I think creating incentives to review is going to be more potent and
> > more enjoyable for everyone involved than punitive measures.
> >
> >
> >
>
> Indeed. I once suggested only half jokingly that we should have a "Coder
> of the month" award. Maybe a "Reviewer of the month" award would also be
> good. Seriously, the major benefit most people get from contributing
> (apart from good karma and a warm inner glow) is kudos, and we should
> possibly lay it on a bit thicker.

In the old days (I can't believe I said that), it was not uncommon for a
developer to just want a thank you, some pizza and beer. I don't know
that it is much different now.

It is amazing what people are willing to do if they feel a little
appreciated.

Joshua D. Drake

>
> cheers
>
> andrew
>

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander

On Nov 16, 2009, at 8:47 PM, "Joshua D. Drake" <jd(at)commandprompt(dot)com>
wrote:

> On Mon, 2009-11-16 at 19:15 -0500, Andrew Dunstan wrote:
>>
>> Brendan Jurd wrote:
>>> One of the rewards for getting a patch into the tree is having your
>>> name immortalised in the commit log. There's no such compensation
>>> for
>>> reviewing patches.
>>>
>>> I think creating incentives to review is going to be more potent and
>>> more enjoyable for everyone involved than punitive measures.
>>>
>>>
>>>
>>
>> Indeed. I once suggested only half jokingly that we should have a
>> "Coder
>> of the month" award. Maybe a "Reviewer of the month" award would
>> also be
>> good. Seriously, the major benefit most people get from contributing
>> (apart from good karma and a warm inner glow) is kudos, and we should
>> possibly lay it on a bit thicker.
>
> In the old days (I can't believe I said that), it was not uncommon
> for a
> developer to just want a thank you, some pizza and beer. I don't know
> that it is much different now.
>
> It is amazing what people are willing to do if they feel a little
> appreciated.

+1.

...Robert
>
>>

On Mon, Nov 16, 2009 at 6:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Brendan Jurd <direvus(at)gmail(dot)com> writes:
>> One of the rewards for getting a patch into the tree is having your
>> name immortalised in the commit log.  There's no such compensation for
>> reviewing patches.
>
> Well, that could be fixed: instead of
>
>        blah blah blah
>
>        Joe Coder
>
> we could write
>
>        blah blah blah
>
>        Joe Coder, reviewed by X and Y
>
> Although keeping track of just who to credit might be a bit tricky.
> I'd be happy to commit to crediting whoever is listed in the CF entry
> for the patch, but sometimes other people have chimed in as much or
> more as the nominal reviewer.

If looking at the CF entries, it's important to note that sometimes
one person posts a review and someone else (historically, me) adds a
link to it, and of course we want to post the reviewer, not the person
who dropped in the link. I try to always make the comment something
like "review from so-and-so" when I do this, but I (or someone) might
forget that on occasion, so clicking through to the underlying message
is probably a good idea.

As for other people chiming in, I think half a loaf is better than
none. We should try to credit the people who deserve credit; and if
someone who chimes in is particularly concerned about getting credit,
then they can post a link to their chiming-in on the CF app.
Otherwise, it's best effort, just like anything else.

...Robert

On Nov 17, 2009, at 9:15 AM, Andrew Dunstan wrote:

> Indeed. I once suggested only half jokingly that we should have a "Coder of the month" award.

I suggest that it be named "The Tom Lane" award, and disqualify Tom from winning (sorry Tom). ;-)

David