Thread: [9.4 CF 1] The Commitfest Slacker List
Folks, For 9.2, we adopted it as policy that anyone submitting a patch to a commitfest is expected to review at least one patch submitted by someone else. And that failure to do so would affect the attention your patches received in the future. For that reason, I'm publishing the list below of people who submitted patches and have not yet claimed any patch in the commitfest to review. For those of you who are contributing patches for your company, please let your boss know that reviewing is part of contributing, and that if you don't do the one you may not be able to do the other. The two lists below, idle submitters and committers, constitutes 26 people. This *outnumbers* the list of contributors who are busy reviewing patches -- some of them four or more patches. If each of these people took just *one* patch to review, it would almost entirely clear the list of patches which do not have a reviewer. If these folks continue not to do reviews, this commitfest will drag on for at least 2 weeks past its closure date. Andrew Geirth Nick White Peter Eisentrout Alexander Korotkov Etsuro Fujita Hari Babu Jameison Martin Jon Nelson Oleg Bartunov Chris Farmiloe Samrat Revagade Alexander Lakhin Mark Kirkwood Liming Hu Maciej Gajewski Josh Kuperschmidt Mark Wong Gurjeet Singh Robins Tharakan Tatsuo Ishii Karl O Pinc Additionally, the following committers are not listed as reviewers on any patch. Note that I have no way to search which ones might be *committers* on a patch, so these folks are not necessarily slackers (although with Bruce, we know for sure): Bruce Momjian (momjian) Michael Meskes (meskes) Teodor Sigaev (teodor) Andrew Dunstan (adunstan) Magnus Hagander (mha) Itagaki Takahiro (itagaki) -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 06/24/2013 12:41 AM, Josh Berkus wrote: > Folks, > > For 9.2, we adopted it as policy that anyone submitting a patch to a > commitfest is expected to review at least one patch submitted by someone > else. And that failure to do so would affect the attention your patches > received in the future. For that reason, I'm publishing the list below > of people who submitted patches and have not yet claimed any patch in > the commitfest to review. > > For those of you who are contributing patches for your company, please > let your boss know that reviewing is part of contributing, and that if > you don't do the one you may not be able to do the other. > > The two lists below, idle submitters and committers, constitutes 26 > people. This *outnumbers* the list of contributors who are busy > reviewing patches -- some of them four or more patches. If each of > these people took just *one* patch to review, it would almost entirely > clear the list of patches which do not have a reviewer. If these folks > continue not to do reviews, this commitfest will drag on for at least 2 > weeks past its closure date. > > Andrew Geirth > Nick White > Peter Eisentrout > Alexander Korotkov > Etsuro Fujita > Hari Babu > Jameison Martin > Jon Nelson > Oleg Bartunov > Chris Farmiloe > Samrat Revagade > Alexander Lakhin > Mark Kirkwood > Liming Hu > Maciej Gajewski > Josh Kuperschmidt > Mark Wong > Gurjeet Singh > Robins Tharakan > Tatsuo Ishii > Karl O Pinc > > Additionally, the following committers are not listed as reviewers on > any patch. Note that I have no way to search which ones might be > *committers* on a patch, so these folks are not necessarily slackers > (although with Bruce, we know for sure): > > Bruce Momjian (momjian) > Michael Meskes (meskes) > Teodor Sigaev (teodor) > Andrew Dunstan (adunstan) > Magnus Hagander (mha) > Itagaki Takahiro (itagaki) > I think we maybe need to be a bit more careful about a name and shame policy, or it will be ignored. I actually reviewed Peter's emacs config patch just yesterday, although I didn't note that on the Commitfest app. (Incidentally, based on Tom's later comments I think we should probably mark that one as rejected.) Anyway, I have claimed the VPATH patches for review, and will look at them today. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > I think we maybe need to be a bit more careful about a name and shame > policy, or it will be ignored. I very much don't like that idea of publishing a list of names either. Editing the reviewer field and sending personal notices is fine by me, but name and shame is walking the line… Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Maybe this policy should be mentioned on the Wiki, so newbies like myself (who wouldn't even dare reviewing patches submitted be seasoned hackers) are not surprised by seeing own name on a shame wall?
M
On 06/24/2013 05:40 PM, Maciej Gajewski wrote: > Maybe this policy should be mentioned on the Wiki, so newbies like > myself (who wouldn't even dare reviewing patches submitted be seasoned > hackers) are not surprised by seeing own name on a shame wall? I personally would prefer if the email was sent to those who should be reminded instead to the list. My personal experience is that personal reminders are more effective than public naming and shaming. -- Andreas Karlsson
On 06/24/2013 08:40 AM, Maciej Gajewski wrote: > Maybe this policy should be mentioned on the Wiki, so newbies like > myself (who wouldn't even dare reviewing patches submitted be seasoned > hackers) are not surprised by seeing own name on a shame wall? It is mentioned. Of course now I can't find it but it is there. However, I believe you are taking the wrong perspective on this. This is not a shame wall. It is a transparent reminder of the policy and those who have not assisted in reviewing a patch but have submitted a patch themselves. In short, leave the ego at the door. Sincerely, JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
"Joshua D. Drake" <jd@commandprompt.com> writes: > In short, leave the ego at the door. That's not the problem. Let's welcome those who are able to contribute their time and skills without making it harder for them. Motivation here shoulnd't be how to avoid getting enlisted on the shame wall. My opinion is that if we play the game that way, we will only achieve to push contributors out of the community and review process. Please kindly reconsider and don't publish any such backward list again. Instead, I don't know, fetch some SPI money to offer a special poster or unique one-time-edition only hoodie or a signed mug or whatever to extra proficient contributors and turn that into a game people want to win. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 06/24/2013 05:54 PM, Joshua D. Drake wrote: > > On 06/24/2013 08:40 AM, Maciej Gajewski wrote: >> Maybe this policy should be mentioned on the Wiki, so newbies like >> myself (who wouldn't even dare reviewing patches submitted be seasoned >> hackers) are not surprised by seeing own name on a shame wall? > > It is mentioned. Of course now I can't find it but it is there. > > However, I believe you are taking the wrong perspective on this. This > is not a shame wall. It is a transparent reminder of the policy and > those who have not assisted in reviewing a patch but have submitted a > patch themselves. > > In short, leave the ego at the door. Would be much easier to do if you did not post this to the list :) I guess you can also extract the e-mails and post only to the "slackers". -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ
On 06/24/2013 08:01 AM, Dimitri Fontaine wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> I think we maybe need to be a bit more careful about a name and shame >> policy, or it will be ignored. > > I very much don't like that idea of publishing a list of names either. > Editing the reviewer field and sending personal notices is fine by me, > but name and shame is walking the line… Actually, every submitter on that list -- including Maciej -- was sent a personal, private email a week ago. A few (3) chose to take the opportunity to review things, or promised to do so, including a brand new Chinese contributor who needed help with English to review his patch. The vast majority chose not to respond to my email to them at all. When private email fails, the next step is public email. Maciej is correct that this policy also belongs on the "how to submit a patch" wiki page. I will remedy that. Doing the patch counts yesterday, it became clear to me that the reason for the patch review pileups was that many people were submitting patches but not participating in the review process at all. That is, we have 100 to 150 people a year submitting patches, but relying entirely on the committers and a few heroic uber-reviewers to do 90% of the patch review. This is the commitfest problem in a nutshell. The purpose of the list was to make it completely apparent where the problem in clearing the patch queue lies, and to get some of our submitters to do patch review. Per both of my emails yesterday, I am trying to make sure that this CF finishes on time. Following the rules passed at the developer meetings for how CFs are to be run, I am doing so. If the result is unsatisfactory, I can always resign as CFM. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Josh Berkus <josh@agliodbs.com> writes: > patch. The vast majority chose not to respond to my email to them at > all. When private email fails, the next step is public email. The only problem I have here is that I don't remember about deciding to publish a list of failures by public email at all. I hope it's not my memory failing me here, because then I would have to remember why I didn't speak up against that idea at the time. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 06/24/2013 10:02 AM, Dimitri Fontaine wrote: > Josh Berkus <josh@agliodbs.com> writes: >> patch. The vast majority chose not to respond to my email to them at >> all. When private email fails, the next step is public email. > > The only problem I have here is that I don't remember about deciding to > publish a list of failures by public email at all. I hope it's not my > memory failing me here, because then I would have to remember why I > didn't speak up against that idea at the time. You didn't decide anything. As the CFM, I did. My job for this month is to make sure that 100% of patches in that queue get reviewed and either committed or bounced by July 15th. I'm doing my job. I will be more than happy to resign as CFM and turn it over to someone else if people have a problem with it. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 2013-06-24 10:10:11 -0700, Josh Berkus wrote: > On 06/24/2013 10:02 AM, Dimitri Fontaine wrote: > > Josh Berkus <josh@agliodbs.com> writes: > >> patch. The vast majority chose not to respond to my email to them at > >> all. When private email fails, the next step is public email. > > > > The only problem I have here is that I don't remember about deciding to > > publish a list of failures by public email at all. I hope it's not my > > memory failing me here, because then I would have to remember why I > > didn't speak up against that idea at the time. > > You didn't decide anything. As the CFM, I did. My job for this month > is to make sure that 100% of patches in that queue get reviewed and > either committed or bounced by July 15th. I'm doing my job. > > I will be more than happy to resign as CFM and turn it over to someone > else if people have a problem with it. Heck, Josh. People have to be allowed to critize *a small part* of your work without you understanding it as a fundamental request to step back from being CFM. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
> Instead, I don't know, fetch some SPI money to offer a special poster or > unique one-time-edition only hoodie or a signed mug or whatever to extra > proficient contributors and turn that into a game people want to win. I like that idea too. Provided that we allocate enough funding that I can have a paid admin handle the shipping etc. Frankly, I'd be up for the idea that patch reviewers get a special t-shirt for each PostgresQL release, for reviewing even *one* patch. Mind you, we wouldn't be able to reward a few reviewers, because they live in countries to which it's impossible to ship from abroad. I have previously proposed that all of the reviewers of a given PostgreSQL release be honored in the release notes as a positive incentive, and was denied on this from doing so. Not coincidentally, we don't seem to have any reviewers-at-large anymore. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 06/24/2013 10:10 AM, Josh Berkus wrote: > > On 06/24/2013 10:02 AM, Dimitri Fontaine wrote: >> Josh Berkus <josh@agliodbs.com> writes: >>> patch. The vast majority chose not to respond to my email to them at >>> all. When private email fails, the next step is public email. >> >> The only problem I have here is that I don't remember about deciding to >> publish a list of failures by public email at all. I hope it's not my >> memory failing me here, because then I would have to remember why I >> didn't speak up against that idea at the time. > > You didn't decide anything. As the CFM, I did. My job for this month > is to make sure that 100% of patches in that queue get reviewed and > either committed or bounced by July 15th. I'm doing my job. > > I will be more than happy to resign as CFM and turn it over to someone > else if people have a problem with it. Let's not be hasty :) I think JoshB is spot on in this. He sent previous private emails, and a week later opened up the transparency so that everyone understood what was going on. What I find unfortunate is people are spending a bunch of time on this argument which has been clearl thought out by Josh instead of reviewing patches. I repeat: Leave your ego at the door. Josh is doing what could be considered one of the most thankless (public) jobs in this project. How about we support him in getting these patches taken care of instead of whining about the fact that he called us out for not doing our jobs (reviewing patches) in the first place. Sincerely, Joshua D. Drkae -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
>> I will be more than happy to resign as CFM and turn it over to someone >> else if people have a problem with it. > > Heck, Josh. People have to be allowed to critize *a small part* of your > work without you understanding it as a fundamental request to step back > from being CFM. Criticize, yes. Which I'm responding to. But if this group votes that I am not to publish a public list of submitters who aren't reviewing again, or otherwise votes to restrict my ability to enforce the rules which the Developer Meetings have decided on for CFs, I will resign as CFM. The job is too hard to do with one hand tied behind my back. That's what I'm saying. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Mon, Jun 24, 2013 at 10:54 PM, Josh Berkus <josh@agliodbs.com> wrote: > >>> I will be more than happy to resign as CFM and turn it over to someone >>> else if people have a problem with it. >> >> Heck, Josh. People have to be allowed to critize *a small part* of your >> work without you understanding it as a fundamental request to step back >> from being CFM. > > Criticize, yes. Which I'm responding to. > > But if this group votes that I am not to publish a public list of > submitters who aren't reviewing again, or otherwise votes to restrict my > ability to enforce the rules which the Developer Meetings have decided > on for CFs, I will resign as CFM. The job is too hard to do with one > hand tied behind my back. That's what I'm saying. Hi Josh, Not sure if this is out of line, but I would be glad to help you out in any way possible for this CF. Please let me know if I can lighten your burden in any way. Regards, Atri -- Regards, Atri l'apprenant
On Mon, Jun 24, 2013 at 2:22 PM, Josh Berkus <josh@agliodbs.com> wrote: > I have previously proposed that all of the reviewers of a given > PostgreSQL release be honored in the release notes as a positive > incentive, and was denied on this from doing so. Not coincidentally, we > don't seem to have any reviewers-at-large anymore. You know... that's a very good idea.
On 06/24/2013 10:22 AM, Josh Berkus wrote: > Mind you, we wouldn't be able to reward a few reviewers, because they > live in countries to which it's impossible to ship from abroad. > > I have previously proposed that all of the reviewers of a given > PostgreSQL release be honored in the release notes as a positive > incentive, and was denied on this from doing so. Not coincidentally, we > don't seem to have any reviewers-at-large anymore. I don't like idea of sending gifts. I do like the idea of public thanks. We should put full recognition in the release notes for someone who reviews a patch. If they didn't review the patch, the person that wrote the patch would not have gotten the patch committed anyway. Writing the patch is only have the battle. Heck, think about the FKLocks patch, Alvaro wrote that patch but I know that Noah (as well as others) put a herculean effort into helping get it committed. Reviewer recognition should be on the same level as the submitter. JD > -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
On 2013-06-24 10:37:02 -0700, Joshua D. Drake wrote: > > On 06/24/2013 10:22 AM, Josh Berkus wrote: > > >Mind you, we wouldn't be able to reward a few reviewers, because they > >live in countries to which it's impossible to ship from abroad. > > > >I have previously proposed that all of the reviewers of a given > >PostgreSQL release be honored in the release notes as a positive > >incentive, and was denied on this from doing so. Not coincidentally, we > >don't seem to have any reviewers-at-large anymore. > > I don't like idea of sending gifts. I do like the idea of public thanks. We > should put full recognition in the release notes for someone who reviews a > patch. If they didn't review the patch, the person that wrote the patch > would not have gotten the patch committed anyway. Writing the patch is only > have the battle. > > Heck, think about the FKLocks patch, Alvaro wrote that patch but I know that > Noah (as well as others) put a herculean effort into helping get it > committed. > > Reviewer recognition should be on the same level as the submitter. The problem with that is that that HUGELY depends on the patch and the review. There are patches where reviewers do a good percentage of the work and others where they mostly tell that "compiles & runs". Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
JD said: > Leave your ego at the door. Josh is doing what could be considered one > of the most thankless (public) jobs in this project. How about we > support him in getting these patches taken care of instead of whining > about the fact that he called us out for not doing our jobs (reviewing > patches) in the first place. Actually, I think this is a very important thing for us to discuss, in fact more important than reviewing any individual patch. 9.3 CF4 took almost **4 months** so it's clear that the system isn't working as designed. So let's hash this out during CF1 rather than during CF4. We've had the policy of "submit one, review one" since the 9.2 dev cycle. However, to my knowledge nobody has actually tried to enforce this before, or even published stats on it. Once I did that for this CF, it became readily apparent to me that the failure of this policy is at least 50% of the problem with finishing commitfests -- and releases -- on time. The vast majority of submitters aren't reviewing other people's patches, even ones who have the time and resources to do so. You'll notice that most of the people on the List aren't new contributors to PostgreSQL; if anything, the new contributors have been exemplary in responding to private email that they need to do some review. More, on the slacker list are 6-8 people who I happen to know are paid by their employers to work on PostgreSQL. Those are the folks I'm particularly targeting with the Slacker list; I want to make it transparently clear to those folks' bosses that they have to give their staff time for patch review if they expect to get the features *they* want into PostgreSQL. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Mon, Jun 24, 2013 at 12:57 PM, Josh Berkus <josh@agliodbs.com> wrote: > Actually, every submitter on that list -- including Maciej -- was sent a > personal, private email a week ago. A few (3) chose to take the > opportunity to review things, or promised to do so, including a brand > new Chinese contributor who needed help with English to review his > patch. The vast majority chose not to respond to my email to them at > all. When private email fails, the next step is public email. Hrm, I'm on the slackers list, and I didn't see an email directed to me from JB in the last week about the CF. Anyway, I am hoping to take at least one patch this CF, though the recent "review it within 5 days or else" policy combined with a ton of my own work has kept me back so far. Josh
On 24 June 2013 18:10, Josh Berkus <josh@agliodbs.com> wrote: > I will be more than happy to resign as CFM and turn it over to someone > else if people have a problem with it. Please don't do that (until at least the end of the CF ;-) ) It's a difficult job and I'm happy you're doing it, though I suggest an optimal setting of 2-3 is likely to work best: http://en.wikipedia.org/wiki/Mean_Machine_Angel I would guess that many people probably don't actually understand the problems or the policies that have emerged as a result. Anyway, I won't say anymore because I'll probably be on some list or other myself sometime. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
> Hrm, I'm on the slackers list, and I didn't see an email directed to > me from JB in the last week about the CF. Really? Hmmm. I'm going to send you a test email privately, please verify whether or not you get it. > Anyway, I am hoping to take at least one patch this CF, though the > recent "review it within 5 days or else" policy combined with a ton of > my own work has kept me back so far. Thanks! Your help is much appreciated, on whatever schedule you can do it! -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Mon, Jun 24, 2013 at 2:41 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> I don't like idea of sending gifts. I do like the idea of public thanks. We >> should put full recognition in the release notes for someone who reviews a >> patch. If they didn't review the patch, the person that wrote the patch >> would not have gotten the patch committed anyway. Writing the patch is only >> have the battle. >> >> Heck, think about the FKLocks patch, Alvaro wrote that patch but I know that >> Noah (as well as others) put a herculean effort into helping get it >> committed. >> >> Reviewer recognition should be on the same level as the submitter. > > The problem with that is that that HUGELY depends on the patch and the > review. There are patches where reviewers do a good percentage of the > work and others where they mostly tell that "compiles & runs". Well, you can't so arbitrarily pick who you're recognizing as contributor and who you aren't. So why not mention them all? They did work for it, some more than others, but they all worked. And since whoever submitted a patch (and got it committed) must have reviewed something as well, they'd be recognized for both reviewing and submitting.
> The problem with that is that that HUGELY depends on the patch and the > review. There are patches where reviewers do a good percentage of the > work and others where they mostly tell that "compiles & runs". This project is enormously stingy with giving credit to people. It's not like it costs us money, you know. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 06/24/2013 10:48 AM, Claudio Freire wrote: >>> Reviewer recognition should be on the same level as the submitter. >> >> The problem with that is that that HUGELY depends on the patch and the >> review. There are patches where reviewers do a good percentage of the >> work and others where they mostly tell that "compiles & runs". > > > Well, you can't so arbitrarily pick who you're recognizing as > contributor and who you aren't. So why not mention them all? They did > work for it, some more than others, but they all worked. And since > whoever submitted a patch (and got it committed) must have reviewed > something as well, they'd be recognized for both reviewing and > submitting. > Exactly. Just make it a simple policy: Submitters and Reviewers are listed in that order: Submitter, reviewer, reviewer That way submitter gets first bill, satisfying the ego (as well as professional consideration) but reviewers are also fully recognized. Sincerely, Joshua D. Drkae -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
On 2013-06-24 14:48:32 -0300, Claudio Freire wrote: > On Mon, Jun 24, 2013 at 2:41 PM, Andres Freund <andres@2ndquadrant.com> wrote: > >> I don't like idea of sending gifts. I do like the idea of public thanks. We > >> should put full recognition in the release notes for someone who reviews a > >> patch. If they didn't review the patch, the person that wrote the patch > >> would not have gotten the patch committed anyway. Writing the patch is only > >> have the battle. > >> > >> Heck, think about the FKLocks patch, Alvaro wrote that patch but I know that > >> Noah (as well as others) put a herculean effort into helping get it > >> committed. > >> > >> Reviewer recognition should be on the same level as the submitter. > > > > The problem with that is that that HUGELY depends on the patch and the > > review. There are patches where reviewers do a good percentage of the > > work and others where they mostly tell that "compiles & runs". > > > Well, you can't so arbitrarily pick who you're recognizing as > contributor and who you aren't. So why not mention them all? They did > work for it, some more than others, but they all worked. And since > whoever submitted a patch (and got it committed) must have reviewed > something as well, they'd be recognized for both reviewing and > submitting. Because spending a year working on a feature isn't the same as spending an hour or day on it. And the proposal was to generally list them at the same level. At least the 9.3 release notes seem to list people that reviewed extensively prominently on the patches... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
> Because spending a year working on a feature isn't the same as spending > an hour or day on it. And the proposal was to generally list them at the > same level. > At least the 9.3 release notes seem to list people that reviewed > extensively prominently on the patches... My proposal was to have a compiled list of reviewers at the end of the release notes, in the form of: "Reviewers and Testers for #.# Included: name, name, name, name" That way we can pick up the trivial reviewers as well, and even testers who are not otherwise contributors. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 2013-06-24 10:50:42 -0700, Josh Berkus wrote: > > > The problem with that is that that HUGELY depends on the patch and the > > review. There are patches where reviewers do a good percentage of the > > work and others where they mostly tell that "compiles & runs". > > This project is enormously stingy with giving credit to people. It's > not like it costs us money, you know. Listing a reviewer that didn't do all that much at the same level as the author or an somebody having done an extensive review will cost you contributors in the long run. I am all for introducing a "Contributed by reviewing: ..." section in the release notes. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 06/24/2013 10:59 AM, Andres Freund wrote: > > On 2013-06-24 10:50:42 -0700, Josh Berkus wrote: >> >>> The problem with that is that that HUGELY depends on the patch and the >>> review. There are patches where reviewers do a good percentage of the >>> work and others where they mostly tell that "compiles & runs". >> >> This project is enormously stingy with giving credit to people. It's >> not like it costs us money, you know. > > Listing a reviewer that didn't do all that much at the same level as the > author or an somebody having done an extensive review will cost you > contributors in the long run. > > I am all for introducing a "Contributed by reviewing: ..." section in > the release notes. It should be listed with the specific feature. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
On Mon, Jun 24, 2013 at 1:24 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > Leave your ego at the door. Josh is doing what could be considered one of > the most thankless (public) jobs in this project. How about we support him > in getting these patches taken care of instead of whining about the fact > that he called us out for not doing our jobs (reviewing patches) in the > first place. Quite. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I'm just wondering about newbies...
I've created my first patch, so I'm one of them, I think.
I've reviewed some patches, but only some easier ones, like pure regression tests. Unfortunately my knowledge is not enough to review patches making very deep internal changes, or some efficiency tweaks. I'm not sure if those patches should be reviewed by newbies like me, as I just don't know what is good and what is bad, even if a patch looks OK for me. What's the use of my review, if I don't understand the internals enough, I can apply the patch, run tests, look inside and I'm sure I won't find any problems?
Maybe this is the reason why there are not so many reviewers?
I'm not sure if such a strict policy will bring anything good. If newbies won't be able to review patches, they won't be committing simple patches, as they won't be able to review others.
If this policy will be so strict, I will spend huge amount of time to understand the whole Postgres code before sending my next patch, as most probably I will have problems with making the reviews.
Szymon
Szymon, > I've reviewed some patches, but only some easier ones, like pure regression > tests. Actually, you were one of the people I was thinking of when I said "mostly the new submitters have been exemplary in claiming some review work". You're helping a lot. > Unfortunately my knowledge is not enough to review patches making > very deep internal changes, or some efficiency tweaks. I'm not sure if > those patches should be reviewed by newbies like me, as I just don't know > what is good and what is bad, even if a patch looks OK for me. What's the > use of my review, if I don't understand the internals enough, I can apply > the patch, run tests, look inside and I'm sure I won't find any problems? Actually, something like 50% of all patches get sent back to the submitter on the basis of a build/test/functionality check/completeness check/standards compliance/do we really want this?. The fact that you are doing those steps instead of a committer, and thus letting the committer look at 50% fewer patches, *does* help. In fact, the single most important part of the regression test reviews is "does this new regression test test anything worthwhile?" and "does this regression test return the same results on different machines?". You already know enough to address both of those questions, at least enough to bring up any potential problems on this list. In the future, I would like to have automated systems do the apply/build/regression test check, leaving new reviewers to check only functionality and completeness and other things which require a human. But we don't have the technology for that yet. > Maybe this is the reason why there are not so many reviewers? > > I'm not sure if such a strict policy will bring anything good. If newbies > won't be able to review patches, they won't be committing simple patches, > as they won't be able to review others. Commit a simple patch, review a simple patch. If we have 20 submitters of simple patches, then we have 20 simple patches to review, no? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Mon, Jun 24, 2013 at 10:40:48AM -0700, Josh Berkus wrote: > More, on the slacker list are 6-8 people who I happen to know are paid > by their employers to work on PostgreSQL. Those are the folks I'm > particularly targeting with the Slacker list; I want to make it > transparently clear to those folks' bosses that they have to give their > staff time for patch review if they expect to get the features *they* > want into PostgreSQL. I am confused. Why is everyone interpreting "slacker" negatively. ;-) LOL -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 25/06/13 03:54, Joshua D. Drake wrote: > > > It is mentioned. Of course now I can't find it but it is there. > > However, I believe you are taking the wrong perspective on this. This is > not a shame wall. It is a transparent reminder of the policy and those > who have not assisted in reviewing a patch but have submitted a patch > themselves. > > In short, leave the ego at the door. > Lol - Josh's choice of title has made it a small shame wall (maybe only knee high). However as your last line says - no *actual* harm has been done (no kittens killed etc). One of the reasons for fewer reviewers than submitters, is that it is a fundamentally more difficult job. I've submitted a few patches in a few different areas over the years - however if I grab a patch on the queue that is not in exactly one of the areas I know about, I'll struggle to do a good quality review. Now some might say "any review is better than no review"... I don't think so - one of my patches a while was reviewed by someone who didn't really know the context that well and made the whole process grind to a standstill until a more experienced reviewer took over. I'm quite wary of doing the same myself - anti-help is not the answer! Regards Mark
On Mon, Jun 24, 2013 at 10:10:11AM -0700, Josh Berkus wrote: > On 06/24/2013 10:02 AM, Dimitri Fontaine wrote: > > Josh Berkus <josh@agliodbs.com> writes: > >> patch. The vast majority chose not to respond to my email to them at > >> all. When private email fails, the next step is public email. > > > > The only problem I have here is that I don't remember about deciding to > > publish a list of failures by public email at all. I hope it's not my > > memory failing me here, because then I would have to remember why I > > didn't speak up against that idea at the time. > > You didn't decide anything. As the CFM, I did. My job for this month > is to make sure that 100% of patches in that queue get reviewed and > either committed or bounced by July 15th. I'm doing my job. +1 for trying the management practices you're trying. After the CF is closed, we can step back and treat ourselves to a nice debate about them. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On Tue, Jun 25, 2013 at 11:06 AM, Noah Misch <noah@leadboat.com> wrote: > On Mon, Jun 24, 2013 at 10:10:11AM -0700, Josh Berkus wrote: >> On 06/24/2013 10:02 AM, Dimitri Fontaine wrote: >> > Josh Berkus <josh@agliodbs.com> writes: >> >> patch. The vast majority chose not to respond to my email to them at >> >> all. When private email fails, the next step is public email. >> > >> > The only problem I have here is that I don't remember about deciding to >> > publish a list of failures by public email at all. I hope it's not my >> > memory failing me here, because then I would have to remember why I >> > didn't speak up against that idea at the time. >> >> You didn't decide anything. As the CFM, I did. My job for this month >> is to make sure that 100% of patches in that queue get reviewed and >> either committed or bounced by July 15th. I'm doing my job. > > +1 for trying the management practices you're trying. After the CF is closed, > we can step back and treat ourselves to a nice debate about them. Same here. +1. This method is good to gather in a single, short email all the information a common user cannot see by watching the commitfest page, becoming hard to understand globally with a growing number of pending patches. -- Michael
Mark Kirkwood <mark.kirkwood@catalyst.net.nz> writes: > One of the reasons for fewer reviewers than submitters, is that it is a > fundamentally more difficult job. I've submitted a few patches in a few > different areas over the years - however if I grab a patch on the queue > that is not in exactly one of the areas I know about, I'll struggle to > do a good quality review. > Now some might say "any review is better than no review"... I don't > think so - one of my patches a while was reviewed by someone who didn't > really know the context that well and made the whole process grind to a > standstill until a more experienced reviewer took over. I'm quite wary > of doing the same myself - anti-help is not the answer! FWIW, a large part of the reason for the commitfest structure is that by reviewing patches, people can educate themselves about parts of the PG code that they don't know already, and thus become better qualified to do more stuff later. So I've got no problem with less-experienced people doing reviews. At the same time, it *is* fair to expect someone to phrase their review as "I don't understand this, could you explain and/or improve the comments" rather than saying something more negative, if they aren't clear about what's going on. Without some specific references it's hard to be sure if the reviewer you mention was being unreasonable. Anyway, the point I'm trying to make is that this is a community effort, and each of us can stand to improve our knowledge of what is fundamentally a complex system. Learn something, teach something, it's all good. regards, tom lane
On 25/06/13 15:56, Tom Lane wrote: > Mark Kirkwood <mark.kirkwood@catalyst.net.nz> writes: >> One of the reasons for fewer reviewers than submitters, is that it is a >> fundamentally more difficult job. I've submitted a few patches in a few >> different areas over the years - however if I grab a patch on the queue >> that is not in exactly one of the areas I know about, I'll struggle to >> do a good quality review. > >> Now some might say "any review is better than no review"... I don't >> think so - one of my patches a while was reviewed by someone who didn't >> really know the context that well and made the whole process grind to a >> standstill until a more experienced reviewer took over. I'm quite wary >> of doing the same myself - anti-help is not the answer! > > FWIW, a large part of the reason for the commitfest structure is that > by reviewing patches, people can educate themselves about parts of the > PG code that they don't know already, and thus become better qualified > to do more stuff later. So I've got no problem with less-experienced > people doing reviews. > > At the same time, it *is* fair to expect someone to phrase their review > as "I don't understand this, could you explain and/or improve the > comments" rather than saying something more negative, if they aren't > clear about what's going on. Without some specific references it's hard > to be sure if the reviewer you mention was being unreasonable. > > Anyway, the point I'm trying to make is that this is a community effort, > and each of us can stand to improve our knowledge of what is fundamentally > a complex system. Learn something, teach something, it's all good. > Yes - the reason I mentioned this was not to dig into history and bash a reviewer (who was not at all unreasonable in my recollection)... but to highlight that approaching a review is perhaps a little more complex and demanding that was being made out, hence the shortage of volunteers. However I do completely agree, that encouraging reviewers to proceed with the approach you've outlined above seems like "the way". And yes - it is going to be a good way to get to know the code better. Regards Mark
<div dir="ltr"><br /><div class="gmail_extra"><br /><br /><div class="gmail_quote">On Tue, Jun 25, 2013 at 3:56 PM, Tom Lane<span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span> wrote:<br /><blockquoteclass="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><divclass="im"><br /></div>FWIW, a large part of the reason for the commitfest structureis that<br /> by reviewing patches, people can educate themselves about parts of the<br /> PG code that they don'tknow already, and thus become better qualified<br /> to do more stuff later. So I've got no problem with less-experienced<br/> people doing reviews.<br /><br /> At the same time, it *is* fair to expect someone to phrase theirreview<br /> as "I don't understand this, could you explain and/or improve the<br /> comments" rather than saying somethingmore negative, if they aren't<br /> clear about what's going on. Without some specific references it's hard<br/> to be sure if the reviewer you mention was being unreasonable.<br /><br /> Anyway, the point I'm trying to makeis that this is a community effort,<br /> and each of us can stand to improve our knowledge of what is fundamentally<br/> a complex system. Learn something, teach something, it's all good.<br /><br /> regards, tom lane<br /><div class=""><div class="h5"><br /><br /></div></div></blockquote></div><br /></div><div class="gmail_extra">Ijust wanted to give this the +1 but also want to add. As a novice back in the 8.4 cycle I wrote a smallpatch implement boyer-moore-horspool text searches. I didn't have too much experience around the PostgreSQL source code,so when it came to my review of another patch (which I think was even the rule back in 8.4 IIRC) I was quite clear onwhat I could and could not review. The initial windowing function patch was in the queue at the time, so I picked thatone and reviewed it along with Heikki. As a novice I did manage to help maintain a bit of concurrency with the progressof the patch and the patch went through quite a few revisions from my review even before Heikki got a good look atit. I think the most important thing is maintaining that concurrency during the commitfest, if the author of the patchis sitting idle waiting for a review the whole time then that leaves so much less time to get the patch into shape beforethe commiter comes along. Even if a novice reviewer can only help a tiny bit, it might still make the difference betweenthat patch making it to a suitable state in time or it getting bounced to the next commitfest or even the next release.<br/><br />So for a person who is a little scared to put their name in the reviewer section of a patch, I'd recommendbeing quite open and honest with what you can and can't review. For me back in 8.4 with the windowing function patch,I managed point out a few places were the plans being created where not quite optimal and the author of the patch quicklyput fixes in and sent updated patches, there was also a few things around the SQL spec that I found after grabbinga copy of the spec and reading part of it. It may have been a small part of the overall review and work to get thepatch commited but as Tom stated, I did learn quite a bit from that and I even managed to sent back a delta patch which helped to get the patch more SQL spec compliant.<br /><br />I'm not sure if adding any a review breakdown list to thecommitfest application would be of any use to allow breaking down of what the reviewer is actually reviewing. Perhapspeople would be quicker to sign up to review the sections of patches around their area of expertise rather than puttingtheir name against the whole thing, likely a commiter would have a better idea if such a thing was worth the extraoverhead.<br /><br /></div><div class="gmail_extra">Regards<br /><br />David Rowley<br /></div><div class="gmail_extra"><br/></div></div>
On 25 June 2013 04:13, Joshua D. Drake <jd@commandprompt.com> wrote: > On 06/24/2013 10:59 AM, Andres Freund wrote: >> On 2013-06-24 10:50:42 -0700, Josh Berkus wrote: >>> This project is enormously stingy with giving credit to people. It's >>> not like it costs us money, you know. >> I am all for introducing a "Contributed by reviewing: ..." section in >> the release notes. > > It should be listed with the specific feature. I don't have a strong opinion about whether the reviewers ought to be listed all together or with each feature, but I do feel very strongly that they should be given some kind of credit. Reviewing is often not all that much fun (compared with authoring) and as Josh points out, giving proper credit has a "bang for buck" incentive value that is hard to argue with. Cheers, BJ
Hi,
Apologies for being unable to respond promptly. I've been traveling (without much access) and this was the fastest I could settle down. I was free for months and had to travel smack in the middle of the commitfest.
Apologies for being unable to respond promptly. I've been traveling (without much access) and this was the fastest I could settle down. I was free for months and had to travel smack in the middle of the commitfest.
Incidentally I had reviewed one patch after your direct email, but as someone earlier mentioned, actually pasting my name as 'reviewer' seemed awkward and so didn't
I guess the point Tom mentioned earlier makes good sense and I agree with the
then
(but currently its set to David Fetter for some reason). I guess the point Tom mentioned earlier makes good sense and I agree with the
spirit of the
process.
In my part would try to review more patches and mark them so on the commitfest page soon
.--
Robins Tharakan
On 23 June 2013 23:41, Josh Berkus <josh@agliodbs.com> wrote:
Folks,
For 9.2, we adopted it as policy that anyone submitting a patch to a
commitfest is expected to review at least one patch submitted by someone
else. And that failure to do so would affect the attention your patches
received in the future. For that reason, I'm publishing the list below
of people who submitted patches and have not yet claimed any patch in
the commitfest to review.
For those of you who are contributing patches for your company, please
let your boss know that reviewing is part of contributing, and that if
you don't do the one you may not be able to do the other.
The two lists below, idle submitters and committers, constitutes 26
people. This *outnumbers* the list of contributors who are busy
reviewing patches -- some of them four or more patches. If each of
these people took just *one* patch to review, it would almost entirely
clear the list of patches which do not have a reviewer. If these folks
continue not to do reviews, this commitfest will drag on for at least 2
weeks past its closure date.
Andrew Geirth
Nick White
Peter Eisentrout
Alexander Korotkov
Etsuro Fujita
Hari Babu
Jameison Martin
Jon Nelson
Oleg Bartunov
Chris Farmiloe
Samrat Revagade
Alexander Lakhin
Mark Kirkwood
Liming Hu
Maciej Gajewski
Josh Kuperschmidt
Mark Wong
Gurjeet Singh
Robins Tharakan
Tatsuo Ishii
Karl O Pinc
Additionally, the following committers are not listed as reviewers on
any patch. Note that I have no way to search which ones might be
*committers* on a patch, so these folks are not necessarily slackers
(although with Bruce, we know for sure):
Bruce Momjian (momjian)
Michael Meskes (meskes)
Teodor Sigaev (teodor)
Andrew Dunstan (adunstan)
Magnus Hagander (mha)
Itagaki Takahiro (itagaki)
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Sorry for joining the thread this late, but I didn't really expect to see myself listed as a slacker on a public list. > Additionally, the following committers are not listed as reviewers on > any patch. Note that I have no way to search which ones might be > *committers* on a patch, so these folks are not necessarily slackers This means I'm a slacker because I'm not reviewing or committing a patch, right? Do we have a written rule somewhere? Or some other communication about this? I would have liked to know this requirement before getting singled out in public. Also I'd like to know who made the decision to require a patch review from each committer as I certainly missed it. Was the process public? Where can I find more about it? In general I find it difficult to digest that other people make decisions about my spare time without me having a word in the discussion. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
> Folks, > > For 9.2, we adopted it as policy that anyone submitting a patch to a > commitfest is expected to review at least one patch submitted by someone > else. And that failure to do so would affect the attention your patches > received in the future. For that reason, I'm publishing the list below > of people who submitted patches and have not yet claimed any patch in > the commitfest to review. > > For those of you who are contributing patches for your company, please > let your boss know that reviewing is part of contributing, and that if > you don't do the one you may not be able to do the other. > > The two lists below, idle submitters and committers, constitutes 26 > people. This *outnumbers* the list of contributors who are busy > reviewing patches -- some of them four or more patches. If each of > these people took just *one* patch to review, it would almost entirely > clear the list of patches which do not have a reviewer. If these folks > continue not to do reviews, this commitfest will drag on for at least 2 > weeks past its closure date. > > Andrew Geirth > Nick White > Peter Eisentrout > Alexander Korotkov > Etsuro Fujita > Hari Babu > Jameison Martin > Jon Nelson > Oleg Bartunov > Chris Farmiloe > Samrat Revagade > Alexander Lakhin > Mark Kirkwood > Liming Hu > Maciej Gajewski > Josh Kuperschmidt > Mark Wong > Gurjeet Singh > Robins Tharakan > Tatsuo Ishii > Karl O Pinc It took me for while before realizing that I am on the list because I posted this: http://www.postgresql.org/message-id/20130610.091605.250603479334631505.t-ishii@sraoss.co.jp Because I did not register the patch into CF page myself. I should have not posted it until I find any patch which I can take care of. Sorry for this. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
On Tue, Jul 2, 2013 at 10:52:26AM +0200, Michael Meskes wrote: > Sorry for joining the thread this late, but I didn't really expect to see > myself listed as a slacker on a public list. > > > Additionally, the following committers are not listed as reviewers on > > any patch. Note that I have no way to search which ones might be > > *committers* on a patch, so these folks are not necessarily slackers > > This means I'm a slacker because I'm not reviewing or committing a patch, > right? Do we have a written rule somewhere? Or some other communication about > this? I would have liked to know this requirement before getting singled out in > public. > > Also I'd like to know who made the decision to require a patch review from each > committer as I certainly missed it. Was the process public? Where can I find > more about it? In general I find it difficult to digest that other people make > decisions about my spare time without me having a word in the discussion. I understand. You could wear "slacker" as a badge of honor: ;-) http://momjian.us/main/img/main/slacker.jpg -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 07/02/2013 11:30 AM, Tatsuo Ishii wrote: >> Folks, >> >> For 9.2, we adopted it as policy that anyone submitting a patch to a >> commitfest is expected to review at least one patch submitted by someone >> else. And that failure to do so would affect the attention your patches >> received in the future. For that reason, I'm publishing the list below >> of people who submitted patches and have not yet claimed any patch in >> the commitfest to review. >> >> For those of you who are contributing patches for your company, please >> let your boss know that reviewing is part of contributing, and that if >> you don't do the one you may not be able to do the other. >> >> The two lists below, idle submitters and committers, constitutes 26 >> people. This *outnumbers* the list of contributors who are busy >> reviewing patches -- some of them four or more patches. If each of >> these people took just *one* patch to review, it would almost entirely >> clear the list of patches which do not have a reviewer. If these folks >> continue not to do reviews, this commitfest will drag on for at least 2 >> weeks past its closure date. >> >> Andrew Geirth >> Nick White >> Peter Eisentrout >> Alexander Korotkov >> Etsuro Fujita >> Hari Babu >> Jameison Martin >> Jon Nelson >> Oleg Bartunov >> Chris Farmiloe >> Samrat Revagade >> Alexander Lakhin >> Mark Kirkwood >> Liming Hu >> Maciej Gajewski >> Josh Kuperschmidt >> Mark Wong >> Gurjeet Singh >> Robins Tharakan >> Tatsuo Ishii >> Karl O Pinc > It took me for while before realizing that I am on the list because I > posted this: > > http://www.postgresql.org/message-id/20130610.091605.250603479334631505.t-ishii@sraoss.co.jp > > Because I did not register the patch into CF page myself. I should > have not posted it until I find any patch which I can take care > of. Hi Tatsuo-san I guess whoever registered it with CF should also take your place on the slackers list ;) Regards Hannu Krosing PS. I am also currently witholding a patch from CF for the same reason > Sorry for this. > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese: http://www.sraoss.co.jp > >
On Tue, Jul 2, 2013 at 3:00 PM, Hannu Krosing <hannu@krosing.net> wrote: > I guess whoever registered it with CF should also take your place on the > slackers list ;) Yeah, I recommend that, in the future, CF managers do NOT go and add patches to the CF. Pinging newbies to see if they just forgot is sensible, but if an experienced hacker hasn't put something in the CF, there's probably a reason. Also, I recommend that nobody get too worked up about being on the slacker list. Life is short, PostgreSQL is awesome, and nobody can make you review patches against your will. Don't take it for more than what Josh meant it as. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013/07/02, at 23:44, Bruce Momjian <bruce@momjian.us> wrote: > I understand. You could wear "slacker" as a badge of honor: ;-) > http://momjian.us/main/img/main/slacker.jpg This picture could make a nice T-shirt btw. > -- Michael
Hackers, Clearly I ticked off a bunch of people by publishing "the list". On the other hand, in the 5 days succeeding the post, more than a dozen additional people signed up to review patches, and we got some of the "ready for committer" patches cleared out -- something which nothing else I did, including dozens of private emails, general pleas to this mailing list, mails to the RRReviewers list, served to accomplish, in this or previous CFs. So, as an experiment, call it a mixed result. I would like to have some other way to motivate reviewers than public shame. I'd like to have some positive motivations for reviewers, such as public recognition by our project and respect from hackers, but I'm doubting that those are actually going to happen, given the feedback I've gotten on this list to the idea. I do think I succeeded in calling attention to the fact that this project has slid into a rut of letting a handful of people do 90% of the reviewing, resulting in CFs which last forever and some very frustrated major contributors. That part shouldn't be necessary again for some time, hopefully. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Tue, Jul 02, 2013 at 04:00:22PM -0400, Robert Haas wrote: > make you review patches against your will. Don't take it for more > than what Josh meant it as. And that was what? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Tue, Jul 02, 2013 at 09:42:43PM -0700, Josh Berkus wrote: > Clearly I ticked off a bunch of people by publishing "the list". On the > other hand, in the 5 days succeeding the post, more than a dozen > additional people signed up to review patches, and we got some of the > "ready for committer" patches cleared out -- something which nothing > else I did, including dozens of private emails, general pleas to this > mailing list, mails to the RRReviewers list, served to accomplish, in > this or previous CFs. So your saying the end justifies the means? I beg to disagree. > So, as an experiment, call it a mixed result. I would like to have some > other way to motivate reviewers than public shame. I'd like to have Doesn't shame imply that people knew that were supposed to review patches in the first place? An implication that is not true, at least for some on your list. I think I better not bring up the word I would describe your email with, just for the fear of mistranslating it. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Wed, Jul 3, 2013 at 9:21 AM, Michael Meskes <meskes@postgresql.org> wrote: > On Tue, Jul 02, 2013 at 04:00:22PM -0400, Robert Haas wrote: >> make you review patches against your will. Don't take it for more >> than what Josh meant it as. > > And that was what? An attempt to prod a few more people into helping review. I can see that this pissed you off, and I'm sorry about that. But I don't think that was his intent. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Clearly I ticked off a bunch of people by publishing "the list". On the > other hand, in the 5 days succeeding the post, more than a dozen > additional people signed up to review patches, and we got some of the > "ready for committer" patches cleared out -- something which nothing > else I did, including dozens of private emails, general pleas to this > mailing list, mails to the RRReviewers list, served to accomplish, in > this or previous CFs. Others rules appeared, like the 5 days limit. To me it outlines that some are abusing the CF app and pushing there useless patches (not still ready or complete, WIP, ...) > So, as an experiment, call it a mixed result. I would like to have some > other way to motivate reviewers than public shame. I'd like to have > some positive motivations for reviewers, such as public recognition by > our project and respect from hackers, but I'm doubting that those are > actually going to happen, given the feedback I've gotten on this list to > the idea. You're looking at a short term, big effect. And long term ? Will people listed still be interested to participate in a project which stamps people ? With or without review, it's a shame if people stop proposing patches because they are not sure to get time to review other things *in time*. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On Wed, Jul 3, 2013 at 2:24 PM, Cédric Villemain <cedric@2ndquadrant.com> wrote:
> Clearly I ticked off a bunch of people by publishing "the list". On theOthers rules appeared, like the 5 days limit.
> other hand, in the 5 days succeeding the post, more than a dozen
> additional people signed up to review patches, and we got some of the
> "ready for committer" patches cleared out -- something which nothing
> else I did, including dozens of private emails, general pleas to this
> mailing list, mails to the RRReviewers list, served to accomplish, in
> this or previous CFs.
To me it outlines that some are abusing the CF app and pushing there useless
patches (not still ready or complete, WIP, ...
Seems to me that "useless" overstates things, but it does seem fair to
say that some patches are not sufficiently well prepared to be efficiently
added into Postgres.
added into Postgres.
> So, as an experiment, call it a mixed result. I would like to have someYou're looking at a short term, big effect.
> other way to motivate reviewers than public shame. I'd like to have
> some positive motivations for reviewers, such as public recognition by
> our project and respect from hackers, but I'm doubting that those are
> actually going to happen, given the feedback I've gotten on this list to
> the idea.
And long term ? Will people listed still be interested to participate in a
project which stamps people ?
With or without review, it's a shame if people stop proposing patches because
they are not sure to get time to review other things *in time*.
Well, if the project is hampered by not being able to get *all* the
changes that people imagine that they want to put in, then we have a
changes that people imagine that they want to put in, then we have a
real problem of needing a sort of "triage" to determine which changes
will be accepted, and which will not.
Perhaps we need an extra status in the CommitFest application, namely
one that characterizes:
one that characterizes:
Insufficiently Important To Warrant Review
That's too long a term. Perhaps "Not Review-worthy" expresses it better?
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"
Tatsuo, > Because I did not register the patch into CF page myself. I should > have not posted it until I find any patch which I can take care > of. Sorry for this. My apologies! I did post the list of patches I'd added to the CF in my "patch sweep" to -hackers, but I forgot to match it against the list of contributors who weren't reviewing. Sorry about that. In general, I prefer to do the patch sweep 5 days out from the start of the CF so that I have time to email people about whether or not their patches should have been included. However, this time an emergency cropped up just before the CF started and I found myself doing the patch sweep the day before the CF, which didn't leave time for an email response. This is one of those areas where better tooling could help a lot. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Michael Meskes wrote: >> So, as an experiment, call it a mixed result. I would like to have some >> other way to motivate reviewers than public shame. I'd like to have > > Doesn't shame imply that people knew that were supposed to review patches in > the first place? An implication that is not true, at least for some on your > list. I think I better not bring up the word I would describe your email with, > just for the fear of mistranslating it. If you didn't feel obligated, you wouldn't be pissed at me. You'd just blow it off (like Bruce did). I think you're angry with me because you feel guilty. My *personal* viewpoint is that all committers should feel obligated to review and commit patches from other contributors. That's why they're committers in the first place. Certainly if a committer looks at the CF application and notices that 80% of the reviewing and committing is being done by three people, none of whom have any more "spare time" than they do, they should feel obligated to help those people out. We have a problem with patch reviewing and committing in this project; it's not being done in a timely fashion in general (every CF last year ended late), and the people who are doing most of the work feel overworked and frustrated. This problem is getting worse every year, and will kill the project if it continues on its current trajectory. There are *only* three ways out of this hole, all three of which I'm trying to address: 1) more automation and better tools in order to reduce the total time required of each reviewer/committer; 2) a program of recruitment of new reviewers, including giving respect and recognition to people for their reviewing efforts 3) getting most of our existing contributors to shoulder their fair share of patch review. (3) is what I'm addressing on this thread. The reason I volunteered to be CFM this time was directly because of our discussion in Ottawa of how the review process wasn't working. I decided to find out *why* it wasn't working, and the first obvious thing I ran across was that most of our current and our long-term contributors weren't doing any patch review. For CF1, the number of people submitting patches outnumbered those who had volunteered for review 2 to 1. That *is* the review problem in a nutshell; everybody wants someone else to do the work. I don't think it's too much to ask people who are listed on the project developers page as major contributors to review one patch per CommitFest most of the time. If they did just *one* it would substantially decrease the workload on the people who are currently doing the vast majority of review and commit. On 07/03/2013 11:24 AM, Cédric Villemain wrote: > You're looking at a short term, big effect. > And long term ? Will people listed still be interested to participate in a > project which stamps people ? > > With or without review, it's a shame if people stop proposing patches because > they are not sure to get time to review other things *in time*. Yes, I am, because the CF is only supposed to be 30 days long, and I plan to finish it on time. That's my job as CFM. Several people on this thread have raised the fear of discouraging patch submitters, but the consistent evidence is that we have more submissions than we can currently handle. I'd rather have half as many submissions, but do a really good job of reviewing, improving, and integrating those than the current mess. Furthermore, there are quite a number of people who are submitting patches on paid company time. For those people, "submit one, review one" has to be an ironclad rule so that they can tell their bosses that they *have* to spend time on patch review. Otherwise, the review doesn't happen. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Le mercredi 3 juillet 2013 21:03:42, Christopher Browne a écrit : > On Wed, Jul 3, 2013 at 2:24 PM, Cédric Villemain <cedric@2ndquadrant.com>wrote: > > > Clearly I ticked off a bunch of people by publishing "the list". On > > > the other hand, in the 5 days succeeding the post, more than a dozen > > > additional people signed up to review patches, and we got some of the > > > "ready for committer" patches cleared out -- something which nothing > > > else I did, including dozens of private emails, general pleas to this > > > mailing list, mails to the RRReviewers list, served to accomplish, in > > > this or previous CFs. > > > > Others rules appeared, like the 5 days limit. > > To me it outlines that some are abusing the CF app and pushing there > > useless > > patches (not still ready or complete, WIP, ... > > Seems to me that "useless" overstates things, but it does seem fair to > say that some patches are not sufficiently well prepared to be efficiently > added into Postgres. Oops! You just made me realized my choice of words was not good at all! I didn't considered under this angle, I just meant that some patches were added to CF to help patches authors, it was a good idea, but this had some unexpected corner case. Sorry for the confusion. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On Wed, Jul 3, 2013 at 12:34:50PM -0700, Josh Berkus wrote: > Michael Meskes wrote: > >> So, as an experiment, call it a mixed result. I would like to have some > >> other way to motivate reviewers than public shame. I'd like to have > > > > Doesn't shame imply that people knew that were supposed to review patches in > > the first place? An implication that is not true, at least for some on your > > list. I think I better not bring up the word I would describe your email with, > > just for the fear of mistranslating it. > > If you didn't feel obligated, you wouldn't be pissed at me. You'd just > blow it off (like Bruce did). I think you're angry with me because you > feel guilty. People are supposed to feel guilty because they are not volunteering enough time, or enough time in the places the community wants? How does that make sense? Doesn't that contradict the term "volunteer"? Also, don't assume everyone has the thick skin I do. I do understand Josh's frustration that something different had to be done. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Wed, Jul 03, 2013 at 09:47:13AM -0400, Robert Haas wrote: > An attempt to prod a few more people into helping review. > > I can see that this pissed you off, and I'm sorry about that. But I > don't think that was his intent. I hoped for this kind of answer from him but ... Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Wed, Jul 03, 2013 at 12:34:50PM -0700, Josh Berkus wrote: > If you didn't feel obligated, you wouldn't be pissed at me. You'd just > blow it off (like Bruce did). I think you're angry with me because you > feel guilty. That is outrageous bullshit! > My *personal* viewpoint is that all committers should feel obligated to And my *personal* viewpoint is that nobody should be offended like this. But apparently I don't get my wish either. > review and commit patches from other contributors. That's why they're > committers in the first place. Certainly if a committer looks at the CF > application and notices that 80% of the reviewing and committing is > being done by three people, none of whom have any more "spare time" than > they do, they should feel obligated to help those people out. How many patches did you review? You don't have to be a committer to do that. > We have a problem with patch reviewing and committing in this project; > it's not being done in a timely fashion in general (every CF last year > ended late), and the people who are doing most of the work feel > overworked and frustrated. This problem is getting worse every year, > and will kill the project if it continues on its current trajectory. As if publicly blaming people for not behaving the way you would like them to will do the project a lot of good. Let me stress again that you didn't even try talking to the people in question in private before, nor did you bother putting your *suggestion* up for discussion before flaming people. Also let me stress again that I did *not* put a patch into the CF. > 3) getting most of our existing contributors to shoulder their fair > share of patch review. > > (3) is what I'm addressing on this thread. The reason I volunteered to > be CFM this time was directly because of our discussion in Ottawa of how > the review process wasn't working. I decided to find out *why* it > wasn't working, and the first obvious thing I ran across was that most > of our current and our long-term contributors weren't doing any patch > review. For CF1, the number of people submitting patches outnumbered > those who had volunteered for review 2 to 1. That *is* the review > problem in a nutshell; everybody wants someone else to do the work. Great, I wasn't part of any discussion as I didn't make it to Ottawa this time. Neither am I part of the problem with 0 patches, but still I've got to shoulder the blame in a less than friendly way. > I don't think it's too much to ask people who are listed on the project > developers page as major contributors to review one patch per CommitFest > most of the time. If they did just *one* it would substantially > decrease the workload on the people who are currently doing the vast > majority of review and commit. You didn't ask! You blamed and offended people! I won't go into details here because frankly why I have no time for reviewing a patch is none of your business. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Wed, Jul 03, 2013 at 04:03:08PM -0400, Bruce Momjian wrote: > I do understand Josh's frustration that something different had to be > done. As a matter of fact I do, too. I just think the style of blaming people in public like this is not ideal. As I said I didn't even notice this email in the first hand until I was approached from other people and called a slacker in communication not related to the CF at all. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On 07/03/2013 02:03 PM, Michael Meskes wrote: > I won't go into details here because frankly why I have no time for reviewing a > patch is none of your business. Then just send an email saying "Sorry, I don't have any time for patch review this time. Maybe next time". It's pretty simple. I'm not going to apologize for expecting *committers* to participate in patch review and commit. > As I said I didn't even notice this email in the first hand until I was > approached from other people and called a slacker in communication not related > to the CF at all. Ah, now, *that* wasn't my intent, sorry about that. It's rather a surprise to me that anyone off of the -hackers list would care. Possibly "slacker" was a poor choice of word given translations; in colloquial American English it's a casual term, even affectionate under some conditions. I'll make sure to use different words if I ever end up doing a list again. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 2013-07-03 14:16:09 -0700, Josh Berkus wrote: > On 07/03/2013 02:03 PM, Michael Meskes wrote: > > I won't go into details here because frankly why I have no time for reviewing a > > patch is none of your business. > > Then just send an email saying "Sorry, I don't have any time for patch > review this time. Maybe next time". It's pretty simple. > > I'm not going to apologize for expecting *committers* to participate in > patch review and commit. I find it absurd to expect anybody - including committers and regular contributors - to be involved in the project all the time. It's one thing to call somebody out who regularly commits his/her own stuff but doesn't do CF work, something entirely different to do it to people who didn't have time (or even chose to invest it differently!) to contribute lately. The project does *NOT* own them. I'd be at least as pissed as Michael seems to be if I were in his shoes. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jul 3, 2013 at 5:16 PM, Josh Berkus <josh@agliodbs.com> wrote: > I'm not going to apologize for expecting *committers* to participate in > patch review and commit. You are way out of line. You have no right to expect ANYONE to participate in patch review and commit. Michael is doing us a favor by maintaining ECPG even though he's not heavily involved in the project any more and has other things to do with his time. I think you're about two emails away from him having him resign in disgust, and if he does, then the burden of reviewing and committing ECPG patches is going to fall on someone else. Do you expect Tom or Noah or Simon or myself to pick up the slack after you've driven him away? I suppose you probably do, and that is absolutely wrong and really pretty offensive. I think it's completely appropriate for you to remind people who have submitted patches for review but not reviewed any that they need to do that part of it, too. Fair is fair. But you cannot enforce mandatory volunteerism on people just because they are committers. Maybe you think the world would be a better place if committers who didn't pull their weight had their commit bits pulled, or that it doesn't matter if you drive them to resign in disgust. I respectfully disagree. Yeah, committers who are completely idle and never do anything probably shouldn't have a commit bit. But someone like Michael who reliably maintains ECPG is an asset to the project whether he chooses to do anything else or not. I'm flabbergasted that you can't see that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 07/03/2013 03:08 PM, Robert Haas wrote: > You are way out of line. You have no right to expect ANYONE to > participate in patch review and commit. Michael is doing us a favor > by maintaining ECPG even though he's not heavily involved in the > project any more and has other things to do with his time. That's a good point. I hadn't considered (or realized the extent of) the occasional and specific nature of Michael's involvement with the project these days. My apologies, then, Michael. Is there anyone else on the committer list with similar circumstances? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Wed, Jul 3, 2013 at 6:34 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 07/03/2013 03:08 PM, Robert Haas wrote: >> You are way out of line. You have no right to expect ANYONE to >> participate in patch review and commit. Michael is doing us a favor >> by maintaining ECPG even though he's not heavily involved in the >> project any more and has other things to do with his time. > > That's a good point. I hadn't considered (or realized the extent of) > the occasional and specific nature of Michael's involvement with the > project these days. My apologies, then, Michael. > > Is there anyone else on the committer list with similar circumstances? I would say that everyone on the committer list and every other list has the right to choose the level of their involvement. We can't really complain about what people choose to do or not do except to the extent that they impose burdens on other people. For example, we typically expect that if a committer commits a patch that breaks something, that committer will promptly fix it. People who are not willing to do that should not commit (or be allowed to commit). And people who submit patches for review should also review patches: they are asking other people to do work, so they should also contribute work. To the best of my knowledge, no one has ever been made a committer on the condition that they spend a certain minimum number of hours per week, month, or CommitFest on patch review, and I don't think we have any right to expect that they do that. Rather, we should be thanking the people who do choose to do more than their share of patch review, whether they are committers or not. And the only people who need to be called out are people who impose work on others without doing any themselves. People who contribute a small amount but ask nothing for it are a good thing, not a bad thing, again, regardless of whether they have a commit bit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jul 3, 2013 at 03:34:06PM -0700, Josh Berkus wrote: > On 07/03/2013 03:08 PM, Robert Haas wrote: > > You are way out of line. You have no right to expect ANYONE to > > participate in patch review and commit. Michael is doing us a favor > > by maintaining ECPG even though he's not heavily involved in the > > project any more and has other things to do with his time. > > That's a good point. I hadn't considered (or realized the extent of) > the occasional and specific nature of Michael's involvement with the > project these days. My apologies, then, Michael. > > Is there anyone else on the committer list with similar circumstances? I spend my available time going through old emails and finding issues that never made it to a commit-fest, but need doing. I am currently in November, 2012. I am volunteering that information, and do not in any way feel I have to justify my time commitment to anyone, except perhaps my employer. If you want, you can remove my commit bit and I will just post all my patches for others to commit --- hard to see how that is an improvement. I will also remind you that before there were commit-fests, Tom and I pretty much did all that work of committing non-committer's patches. But my big feedback is, our community has no right to be asking about committer circumstances. This is a voluntteer project, and people work as they want. The extrapolation of Josh's approach is that committers have to do work that the community wants to maintain their commit rights, but their commit rights are helping the community, so why would people care if you take them away --- you only hurt the community further by doing so. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
First of all, I'd like to give a big Thank You to all the hackers and slackers that make Postgres great. You've really done an amazing job. I'll step up and take a healthy portion of the blame here. I enjoy the awesome features & fixes that all of you put out year after year, but I have yet to contribute anything. For what it's worth, I'm sorry. If more guys like me could find some time to step up our game a little, we wouldn't even be having this conversation. But we're still left with the fact that there is too much code and not enough review. Honestly, there is a lot of work for committers to do even when all the patches have been through sufficient code review. Slogging through 4 months of CF patches without adequate review is enough to make anybody throw up their hands and quit. The best thing we can do to improve Postgres right now and over the long term is to make sure that doesn't happen. Anybody who is on this list is a valued Postgres contributor, and that's why Josh has been reaching out. Maybe the term slacker offended some people, but he's basically saying that code review is an essential contribution, perhaps more essential than most new patches themselves at this point. He's asking for help from those of you with the skill, experience, and established desire to contribute to the growth of Postgres. Coordinating volunteers for anything is a frustrating process. I'm sure some people on the list shouldn't be on the list, and some people who should be on the list aren't. Maybe the list shouldn't have been sent in the first place. But it's a call for help born out of frustration, and one that we could remedy by stepping up our communication a bit. 5 days is too short? How abut 7 or 10? No time to review a patch this CF? Okay, good to know. He's trying to tell us how we can best contribute. If I manage to find the time to contribute something over the next couple of weeks, it would border on the absurd if my contribution was some sort of patch submission. Pretty much nothing I could write would be a more valuable contribution than code review at this point. So I'll drop Josh an email and let him know how much time I might be able to contribute and when, and I'd suggest other people do the same. Even if it's just a quick "Hey, I'm pretty busy the next couple weeks so I'm not going to be able to review anything this CF." Being armed with a little more information about the potential volunteer pool would probably make his job as CFM much easier. Thanks again to all of you for all your hard work. Best regards, Wolfe Whalen -- Wolfe Whalen wolfe@quios.net
-----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 Josh Berkus replied: >> I won't go into details here because frankly why I have no time >> for reviewing a patch is none of your business. > > Then just send an email saying "Sorry, I don't have any time for patch > review this time. Maybe next time". It's pretty simple. Hope about you not publically shame people in a volunteer project? That's pretty simple. > I'm not going to apologize for expecting *committers* to participate in > patch review and commit. I must have missed the page where patch review is defined as part of a committer's job. > Possibly "slacker" was a poor choice of word given translations; in > colloquial American English it's a casual term, even affectionate under > some conditions. I'll make sure to use different words if I ever end up > doing a list again. Please, don't ever do a list again. And yes, "slacker" was an extremely poor choice of word. This American English speaker certainly has a hard time viewing it as "affectionate". I think the whole thread would have been better received with a subject line of "Commitfest needs help". - -- Greg Sabino Mullane greg@turnstep.com PGP Key: 0x14964AC8 201307032150 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iEYEAREDAAYFAlHU1QQACgkQvJuQZxSWSsgoMgCfcUm/MnYzsUaqVWq3DvTh2kAi sYwAoLAijh3SkCbv2c7visToyqPAOWMG =xoKV -----END PGP SIGNATURE-----
On 04/07/13 10:43, Robert Haas wrote: > And > people who submit patches for review should also review patches: they > are asking other people to do work, so they should also contribute > work. > I think that is an overly simplistic view of things. People submit patches for a variety of reasons, but typically because they think the patch will make the product better (bugfix or new functionality). This is a contribution in itself, not a debt. Now reviewing is performed to ensure that submitted code is *actually* going to improve the product. Both these activities are volunteer work - to attempt to tie them together forceably is unusual to say the least. The skills and experience necessary to review patches are considerably higher than those required to produce patches, hence the topic of this thread. Now I do understand we have a shortage of reviewers and lots of patches, and that this *is* a problem - but what a wonderful problem...many open source projects would love to be in this situation!!! Regards Mark
On Wed, Jul 3, 2013 at 03:34:06PM -0700, Josh Berkus wrote: > On 07/03/2013 03:08 PM, Robert Haas wrote: > > You are way out of line. You have no right to expect ANYONE to > > participate in patch review and commit. Michael is doing us a favor > > by maintaining ECPG even though he's not heavily involved in the > > project any more and has other things to do with his time. > > That's a good point. I hadn't considered (or realized the extent of) > the occasional and specific nature of Michael's involvement with the > project these days. My apologies, then, Michael. > > Is there anyone else on the committer list with similar circumstances? You know what this reminds me of --- early communist movements. Members were scrutinized to see if they were working hard enough for "the cause", and criticized/shamed/punished if they were not. The leaders became tyrants, and justified the tyranny because they were creating "a better society". We all know that didn't end well. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 07/04/2013 09:09 AM, Bruce Momjian wrote: > On Wed, Jul 3, 2013 at 03:34:06PM -0700, Josh Berkus wrote: >> On 07/03/2013 03:08 PM, Robert Haas wrote: >>> You are way out of line. You have no right to expect ANYONE to >>> participate in patch review and commit. Michael is doing us a favor >>> by maintaining ECPG even though he's not heavily involved in the >>> project any more and has other things to do with his time. >> That's a good point. I hadn't considered (or realized the extent of) >> the occasional and specific nature of Michael's involvement with the >> project these days. My apologies, then, Michael. >> >> Is there anyone else on the committer list with similar circumstances? > You know what this reminds me of --- early communist movements. Members > were scrutinized to see if they were working hard enough for "the > cause", and criticized/shamed/punished if they were not. The leaders > became tyrants, and justified the tyranny because they were creating "a > better society". We all know that didn't end well. > I think that's way over the top. Can we all just cool down a bit? I really don't see Josh as Stalin. cheers andrew (who came top of Russian History in his final year)
On Thu, Jul 4, 2013 at 09:16:22AM -0400, Andrew Dunstan wrote: > > On 07/04/2013 09:09 AM, Bruce Momjian wrote: > >On Wed, Jul 3, 2013 at 03:34:06PM -0700, Josh Berkus wrote: > >>On 07/03/2013 03:08 PM, Robert Haas wrote: > >>>You are way out of line. You have no right to expect ANYONE to > >>>participate in patch review and commit. Michael is doing us a favor > >>>by maintaining ECPG even though he's not heavily involved in the > >>>project any more and has other things to do with his time. > >>That's a good point. I hadn't considered (or realized the extent of) > >>the occasional and specific nature of Michael's involvement with the > >>project these days. My apologies, then, Michael. > >> > >>Is there anyone else on the committer list with similar circumstances? > >You know what this reminds me of --- early communist movements. Members > >were scrutinized to see if they were working hard enough for "the > >cause", and criticized/shamed/punished if they were not. The leaders > >became tyrants, and justified the tyranny because they were creating "a > >better society". We all know that didn't end well. > > > > I think that's way over the top. Can we all just cool down a bit? I > really don't see Josh as Stalin. I don't either. It is the "judging others efforts" that concerns me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Jul 04, 2013 at 08:08:57PM +1200, Mark Kirkwood wrote: > On 04/07/13 10:43, Robert Haas wrote: > >> And >> people who submit patches for review should also review patches: they >> are asking other people to do work, so they should also contribute >> work. >> > > I think that is an overly simplistic view of things. People submit > patches for a variety of reasons, but typically because they think the > patch will make the product better (bugfix or new functionality). This > is a contribution in itself, not a debt. True. I don't see that policy as a judgment against the value of submissions, but rather a response ... > Now reviewing is performed to ensure that submitted code is *actually* > going to improve the product. > > Both these activities are volunteer work - to attempt to tie them > together forceably is unusual to say the least. The skills and > experience necessary to review patches are considerably higher than > those required to produce patches, hence the topic of this thread. > > Now I do understand we have a shortage of reviewers and lots of patches, ... to this. Reviewing may be harder than writing a patch, but patch submitters are more promising as reviewers than any other demographic. The situation has a lot in common with the system of academic peer review: http://en.wikipedia.org/wiki/Peer_review#Scholarly_peer_review It's a good value for submitters. By the time my contributions are part of a release, they've regularly become better than I would have achieved working in isolation. Reviewers did that. > and that this *is* a problem - but what a wonderful problem...many open > source projects would love to be in this situation!!! A database is different from much other software in that users build intricate, long-lived software of their own against it. In that respect, it's like the hardware-independent part of a compiler or an OS kernel. When we establish an interface, we maintain it forever or remove it at great user cost. It's also different by virtue of managing long-term state, like a filesystem. That dramatically elevates the potential cost of a bug. A spreadsheet program might reasonably have a different perspective on a surge of submissions. For PostgreSQL, figuring out how to review them is key. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
All, >> I think that's way over the top. Can we all just cool down a bit? I >> really don't see Josh as Stalin. > > I don't either. It is the "judging others efforts" that concerns me. > I agree that publishing the committer portion of the list was a mistake, and will not include it in the future CFM instructions. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Thu, Jul 4, 2013 at 6:16 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >> You know what this reminds me of --- early communist movements. Members >> were scrutinized to see if they were working hard enough for "the >> cause", and criticized/shamed/punished if they were not. The leaders >> became tyrants, and justified the tyranny because they were creating "a >> better society". We all know that didn't end well. >> > > I think that's way over the top. Can we all just cool down a bit? I really > don't see Josh as Stalin. +1. I think that Josh misjudged things in starting this thread, but there is no need for this kind of rhetoric. It helps no one. -- Peter Geoghegan
On Wed, Jul 3, 2013 at 12:03 PM, Christopher Browne <cbbrowne@gmail.com> wrote:
On Wed, Jul 3, 2013 at 2:24 PM, Cédric Villemain <cedric@2ndquadrant.com> wrote:> Clearly I ticked off a bunch of people by publishing "the list". On theOthers rules appeared, like the 5 days limit.
> other hand, in the 5 days succeeding the post, more than a dozen
> additional people signed up to review patches, and we got some of the
> "ready for committer" patches cleared out -- something which nothing
> else I did, including dozens of private emails, general pleas to this
> mailing list, mails to the RRReviewers list, served to accomplish, in
> this or previous CFs.
The limit was previously 4 days (at least according to http://wiki.postgresql.org/wiki/RRReviewers) but I think that that was honored almost exclusively in the breach. I don't have a sense for how the 5 day limit is working. If it is working, great. If not, I would advocate lengthening it--having a limit specified but not generally held to is counterproductive. I know that I, and at least one other potential reviewer, won't ask to be assigned a random patch because we have no confidence we can do an adequate review of a random patch within a contiguous 5 day window.
On the other hand, I could always just go to the open commitfest (rather than the in progress one), pick something at random myself, and have up to 3 months to review it. I just don't do have the discipline to do that, at least not often.
To me it outlines that some are abusing the CF app and pushing there useless
patches (not still ready or complete, WIP, ...Seems to me that "useless" overstates things, but it does seem fair tosay that some patches are not sufficiently well prepared to be efficiently
added into Postgres.
I think that there will always be contributions that need some hand-holding of some form. Isn't that what "returned with feedback" is for?
> So, as an experiment, call it a mixed result. I would like to have someYou're looking at a short term, big effect.
> other way to motivate reviewers than public shame. I'd like to have
> some positive motivations for reviewers, such as public recognition by
> our project and respect from hackers, but I'm doubting that those are
> actually going to happen, given the feedback I've gotten on this list to
> the idea.
And long term ? Will people listed still be interested to participate in a
project which stamps people ?
With or without review, it's a shame if people stop proposing patches because
they are not sure to get time to review other things *in time*.Well, if the project is hampered by not being able to get *all* the
changes that people imagine that they want to put in, then we have areal problem of needing a sort of "triage" to determine which changeswill be accepted, and which will not.Perhaps we need an extra status in the CommitFest application, namely
one that characterizes:Insufficiently Important To Warrant ReviewThat's too long a term. Perhaps "Not Review-worthy" expresses it better?
I don't think that this would really be an improvement. Someone still has to spend enough time looking at the patch to make the decision that it falls into one of those categories. Having spent sufficient time to do that, what they did is ipso facto a review, and they should just set it as either rejected (not important or idea unworkable) or RwF (idea is workable, but the given implementation is not).
Cheers,
Jeff
On 07/05/2013 02:34 PM, Jeff Janes wrote: > On Wed, Jul 3, 2013 at 12:03 PM, Christopher Browne <cbbrowne@gmail.com>wrote: > >> On Wed, Jul 3, 2013 at 2:24 PM, Cédric Villemain <cedric@2ndquadrant.com>wrote: >>> Others rules appeared, like the 5 days limit. >>> >> > The limit was previously 4 days (at least according to > http://wiki.postgresql.org/wiki/RRReviewers) but I think that that was > honored almost exclusively in the breach. I don't have a sense for how the > 5 day limit is working. If it is working, great. If not, I would advocate > lengthening it--having a limit specified but not generally held to is > counterproductive. I know that I, and at least one other potential > reviewer, won't ask to be assigned a random patch because we have no > confidence we can do an adequate review of a random patch within a > contiguous 5 day window. Well, keep in mind that the "reviewer" blank on the commitfest page is solely a reservation to prevent duplicate effort. There is absolutely nothing preventing someone from doing and submitting a review without putting their name down -- and several have, this CF. The idea of the 5-day limit is that this often happens: 1. reviewer takes patch because they want to review it 2. something comes up in their life outside the postgres project, and they don't get time to review it 3. because their name is on the patch as "reviewer", others don't pick it up for review 4. it gets to 3 days before CF end, and nobody has reviewed the patch While there are certainly cases where a reviewer takes more than 5 days actively to review a patch, these are the minority. Most of the time, a reviewer who doesn't review a patch within 5 days doesn't get around to reviewing it that commitfest, period. If you are actively reviewing for more than 5 days, simply send me/Mike an email saying so, before the 5 days are up. Or post something on -hackers in the patch thread. Previously, we dealt with this by sending out an "are you reviewing this patch" email from the CFM to the reviewer after 5 days passed. The problem with that is that a reviewer who was too busy to review was usually too busy to answer, and as a result the whole cycle of "he's not gonna review that" took around 10 days on average before we knew we could remove the name. 10 days is 1/3 of the total commitfest -- unacceptably long. Thus the approach we're trying this CF. Again, this is an area where better automation could really help. Reviewers could get automated reminders at 4 and 5 days, and ack those reminders to extend the review period. > On the other hand, I could always just go to the open commitfest (rather > than the in progress one), pick something at random myself, and have up to > 3 months to review it. I just don't do have the discipline to do that, at > least not often. Please do! >> To me it outlines that some are abusing the CF app and pushing there >>> useless >>> patches (not still ready or complete, WIP, ... >> >> >> Seems to me that "useless" overstates things, but it does seem fair to >> say that some patches are not sufficiently well prepared to be efficiently >> added into Postgres. Unfortunately, the only time we guarantee that a patch or even a spec proposal will get a hearing and discussion is the CF. Thus people who really want to get agreement on a prototype, spec, proposal or API are gonna submit it to the CF, so that they get some useful feedback. Most of the time, someone posting a WIP, API, spec, SQL syntax or feature concept outside of a CF gets little or no useful criticism/suggestions on it. If we don't want WIP/RFC patches in the CF, then we need to provide some other way to guarantee that these incomplete patches will get feedback.I'd be in favor of having something -- I think moreauthors would get better feedback early on in development -- but I have no idea what it would be. Other uncommittable patches submitted to the CF are there because the submitter is sending in a first-time patch. It's very important for training up the new submitter that their patch get the full review-returned-with-feedback cycle. That's how they become better patch authors in the future. Personally, I think this is one of the most valuable aspects of the CF process. >> Well, if the project is hampered by not being able to get *all* the >> changes that people imagine that they want to put in, then we have a >> real problem of needing a sort of "triage" to determine which changes >> will be accepted, and which will not. >> >> Perhaps we need an extra status in the CommitFest application, namely >> one that characterizes: >> Insufficiently Important To Warrant Review Gods forbid. We might as well have the tag "Stupid Idiot Patch" and be done with it. And people accuse *me* of being submitter-hostile. > I don't think that this would really be an improvement. Someone still has > to spend enough time looking at the patch to make the decision that it > falls into one of those categories. Having spent sufficient time to do > that, what they did is ipso facto a review, and they should just set it as > either rejected (not important or idea unworkable) or RwF (idea is > workable, but the given implementation is not). Exactly. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
* Josh Berkus (josh@agliodbs.com) wrote: > Is there anyone else on the committer list with similar circumstances? I'll just flip it around and offer to be publically flogged whenever I'm not helping out with a commitfest. :) Perhaps this should be more "opt-in" than "opt-out", wrt committers anyway. Thanks, Stephen
----- Original Message ----- > * Josh Berkus (josh@agliodbs.com) wrote: > > Is there anyone else on the committer list with similar circumstances? > > I'll just flip it around and offer to be publically flogged whenever I'm > not helping out with a commitfest. :) Perhaps this should be more > "opt-in" than "opt-out", wrt committers anyway. Can we flog you even if you *are* helping? I just wanna see the YouTube video, either way. ;-)
Attachment
On 6/24/13 12:57 PM, Josh Berkus wrote: > Maciej is correct that this policy also belongs on the "how to submit a > patch" wiki page. I will remedy that. I just reviewed and heavily updated the new section you added to https://wiki.postgresql.org/wiki/Submitting_a_Patch That included the idea that the reviewed patch should be similar in size/scope to the submitted one, as well a slightly deeper discussion of how to fit this work into a feature review quote. I find myself needing to explain this whole subject to potential feature sponsors enough that I've tried a few ways of describing it. The closest analog I've found so far is the way "carbon offset" work is accounted for. I sometimes refer to the mutual review as an "offsetting review" in conversation, and I threw that term into the guidelines as well. As far as motivating new reviewers goes, let's talk about positive feedback. Anything that complicates the release notes is a non-starter because that resource is tightly controlled by a small number of people, and it's trying to satisfy a lot of purposes. What I would like to see is an official but simple "Review Leaderboard" for each release instead. Each time someone writes a review and adds it to CF app with a "Review" entry, the account that entered it gets a point. Sum the points at the end and there's your weighted list for T-shirt prizes. It should be possible to get that count with a trivial SELECT query out of the CF database, and then produce a simple HTML table at the end of each CF or release. Anything that takes more work than that, and anything that takes *any* time that must come from a committer, is too much accounting. This idea would be a bit unfair to people who review big patches instead of little ones. But an approach that disproportionately rewards new contributors working on small things isn't that terrible. As long as the review tests whether the code compiles and passes the regression tests, that's good enough to deserve a point. I'd be happy if we suddenly had a problem where people were doing only that to try game their leaderboard ranking. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
On 7/3/13 7:25 PM, Bruce Momjian wrote: > The extrapolation of Josh's approach is that committers > have to do work that the community wants to maintain their commit > rights, but their commit rights are helping the community, so why would > people care if you take them away --- you only hurt the community > further by doing so. The main problem with having inactive committers (which I don't intend to include the important subject matter committers, who I'll get into at the end here) is that they skew the public information about who commits in a counterproductive way. People visit https://wiki.postgresql.org/wiki/Committers , sees that list of names, and then make conclusions based on its content. And some of those conclusions are wrong because the data is inconsistent. The standards listed are applied when new committers are promoted, but they are not applied symmetrically to remove ones who don't anymore. The #1 obstacle to my getting more time to work on core PostgreSQL is that companies presume regular submitters who are also non-committers don't do a very good job. If you are competent and have a consistent track record of contributions to an open source project, the project would make you a committer, right? Conversely, if you've been contributing for a while but aren't a committer, the most likely explanation is that your work quality is poor. That is a completely reasonable viewpoint based on how most open source projects work. The really terrible part is that it means the longer you've been submitting patches, the *less* competent you're assumed to be. When I tell people I've been submitting things since 2007 but am not a committer, the only logical explanation is that my submissions must suck very hard, right? From that perspective, people who are listed as committers but don't actively do work for the project are causing me a serious problem. When someone who rarely commits can obviously qualify, that *proves* the bar for PostgreSQL committers is actually very low to casual observers. That's the message the project is inadvertently sending by leaving committers on there if they stop working actively. The main thing I'd like to see is having the committer list, and its associated guidelines, updated to reflect that there are subject matter experts committing too. That would pull them out of any "what have you done for me lately?" computations, and possibly open up a way to get more of them usefully. Here are the first two obvious labels like that: Michael Meskes (meskes) - embedded SQL Teodor Sigaev (teodor) - inverted indexes When even Josh Berkus doesn't even know all of this information, it's clearly way too obscure to expect the rest of the world to figure it out. It also boggles my mind that there isn't already an entry like this on there too: Thom Browne - documentation Each time Thom passes through yet another correction patch that is committed with no change, I find it downright bizarre that a community with such limited committer resources wastes their time with that gatekeeping. The standards for nominating committers seem based on whether they can commit just about anything. I think it's more important to consider whether people are trusted to keep commits within their known area(s) of expertise. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
Greg, > As far as motivating new reviewers goes, let's talk about positive > feedback. Anything that complicates the release notes is a non-starter > because that resource is tightly controlled by a small number of people, > and it's trying to satisfy a lot of purposes. Greg, you're re-arguing something we obtained a consensus on a week ago. Please read the thread "Kudos for reviewers". -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com