Thread: next CommitFest
The next CommitFest is scheduled to start in a week. So far, it doesn't look too bad, though a lot could change between now and then. I would personally prefer not to be involved in the management of the next CommitFest. Having done all of the July CommitFest and a good chunk of the September CommitFest, I am feeling a bit burned out. One pretty major fly in the ointment is that neither Hot Standby nor Streaming Replication has been committed or shows much sign of being about to be committed. I think this is bad. These are big features that figure to have some bugs and break some things. If they're not committed in time for alpha3, then there won't be any significant testing of these prior to alpha4/beta1, at the earliest. I think that's likely to lead to either (1) a very long beta period followed by a late release or (2) a buggy release. I feel like Simon Riggs and Fujii Masao really pulled out all the stops to get these ready in time for the September CommitFest, and while I'm not in a hurry to break the world, I think the sooner these can hit the tree, the better of we'll be in terms of releasing 8.5. Just my $0.02, ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> I would personally prefer not to be involved in the management of the
> next CommitFest. Having done all of the July CommitFest and a good
> chunk of the September CommitFest, I am feeling a bit burned out.
You did yeoman work on both --- thanks for that!
Do we have another volunteer to do this for the November fest?
regards, tom lane
*snip* > One pretty major fly in the ointment is that neither Hot Standby nor > Streaming Replication has been committed or shows much sign of being > about to be committed. I think this is bad. These are big features > that figure to have some bugs and break some things. If they're not > committed in time for alpha3, then there won't be any significant > testing of these prior to alpha4/beta1, at the earliest. I think > that's likely to lead to either (1) a very long beta period followed > by a late release or (2) a buggy release. I feel like Simon Riggs and > Fujii Masao really pulled out all the stops to get these ready in time > for the September CommitFest, and while I'm not in a hurry to break > the world, I think the sooner these can hit the tree, the better of > we'll be in terms of releasing 8.5. > > Just my $0.02, > > absolutely, we should be commit this. we did some testing and things look stable. also, people would most likely want to build code on top of it in be ready for 8.5 (support scripts, etc.). this is important in order to create some acceptance in "user land". this stuffs seems mature and very well thought. just my $0.02 ... regards, hans-jürgen schönig -- Cybertec Schoenig & Schoenig GmbH Reyergasse 9 / 2 A-2700 Wiener Neustadt Web: www.postgresql-support.de
> I would personally prefer not to be involved in the management of the > next CommitFest. Having done all of the July CommitFest and a good > chunk of the September CommitFest, I am feeling a bit burned out. Dave, Selena and I will all be in Japan during the first week of the CF.I can help after that, but during might be hard. Who else helped with the last CF? Is someone else ready to be a CF helper? --Josh Berkus
On Sun, 8 Nov 2009, Robert Haas wrote: > I would personally prefer not to be involved in the management of the > next CommitFest. Having done all of the July CommitFest and a good > chunk of the September CommitFest, I am feeling a bit burned out. I was just poking around on the Wiki, and it looks like the role of the CommitFest manager isn't very well documented yet. Since you've done all of them since introducing the new CF software, I'm not sure if anyone else even knows exactly what you've been doing. The transition over to that was so successful there isn't even a copy of the schedule for 8.5 on the Wiki itself. Could you find some time this week to rattle off an outline of the work involved? It's hard to decide whether to volunteer to help without having a better idea of what's required. -- * Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD
Hi! On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith@gregsmith.com> wrote: > On Sun, 8 Nov 2009, Robert Haas wrote: > >> I would personally prefer not to be involved in the management of the >> next CommitFest. Having done all of the July CommitFest and a good >> chunk of the September CommitFest, I am feeling a bit burned out. > > I was just poking around on the Wiki, and it looks like the role of the > CommitFest manager isn't very well documented yet. Since you've done all of > them since introducing the new CF software, I'm not sure if anyone else even > knows exactly what you've been doing. The transition over to that was so > successful there isn't even a copy of the schedule for 8.5 on the Wiki > itself. Could you find some time this week to rattle off an outline of the > work involved? It's hard to decide whether to volunteer to help without > having a better idea of what's required. It's pretty straightforward. Robert has actually done a great job of communicating about this to the patch reviewers. I'd be happy to get together at some pre-appointed hour this weekend (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already planning to get together to review some patches this Sunday from 3-5pm PST, so that is a convenient time for me. I can also help with commitfest admin tasks, but not in a dedicated way until after Thanksgiving as I'm going to be be traveling or on vacation. -selena -- http://chesnok.com/daily - me http://endpoint.com - work
Selena Deckelmann wrote: > Hi! > > On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith@gregsmith.com> wrote: >> On Sun, 8 Nov 2009, Robert Haas wrote: >> >>> I would personally prefer not to be involved in the management of the >>> next CommitFest. Having done all of the July CommitFest and a good >>> chunk of the September CommitFest, I am feeling a bit burned out. >> I was just poking around on the Wiki, and it looks like the role of the >> CommitFest manager isn't very well documented yet. Since you've done all of >> them since introducing the new CF software, I'm not sure if anyone else even >> knows exactly what you've been doing. The transition over to that was so >> successful there isn't even a copy of the schedule for 8.5 on the Wiki >> itself. Could you find some time this week to rattle off an outline of the >> work involved? It's hard to decide whether to volunteer to help without >> having a better idea of what's required. > > It's pretty straightforward. Robert has actually done a great job of > communicating about this to the patch reviewers. agreed > > I'd be happy to get together at some pre-appointed hour this weekend > (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already > planning to get together to review some patches this Sunday from 3-5pm > PST, so that is a convenient time for me. > > I can also help with commitfest admin tasks, but not in a dedicated > way until after Thanksgiving as I'm going to be be traveling or on > vacation. I would be interested in helping out as well but I won't be able to dedicate a lot of time before that timeframe as well :( Stefan
On Sun, Nov 8, 2009 at 8:52 PM, Robert Haas <robertmhaas@gmail.com> wrote: > The next CommitFest is scheduled to start in a week. So far, it > doesn't look too bad, though a lot could change between now and then. > > I would personally prefer not to be involved in the management of the > next CommitFest. Having done all of the July CommitFest and a good > chunk of the September CommitFest, I am feeling a bit burned out. > why we need a full time manager at all? why not simply use -rrreviewers to track the status of a patch? of course, we hope the author or reviewer to change the status as appropiate but we have seen many people including missing discussions and changing the status of hanging patches... i guess we can make the commitfest app send an email stating that reviewer_foo is taking the patch bar, and maybe sending emails after some days if nothing has happened with that patch... and an email every week or every few days saying how many patches are, how many are being reviewed, how many hasn't been reviewed, and so on... then the remaining work should be not that much, no? -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova <jcasanov@systemguards.com.ec> wrote: > why we need a full time manager at all? > why not simply use -rrreviewers to track the status of a patch? of > course, we hope the author or reviewer to change the status as > appropiate but we have seen many people including missing discussions > and changing the status of hanging patches... Well, actually, that's precisely were I've been putting in a ton of work - making sure patches aren't left hanging. If reviewers would do more of that work, this process would run a lot smoother and be much less onerous for the CM. ...Robert
Selena, > I'd be happy to get together at some pre-appointed hour this weekend > (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already > planning to get together to review some patches this Sunday from 3-5pm > PST, so that is a convenient time for me. Aren't you running OpenSQL this weekend? --Josh
On Wed, Nov 11, 2009 at 4:40 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova > <jcasanov@systemguards.com.ec> wrote: >> why we need a full time manager at all? >> why not simply use -rrreviewers to track the status of a patch? of >> course, we hope the author or reviewer to change the status as >> appropiate but we have seen many people including missing discussions >> and changing the status of hanging patches... > > Well, actually, that's precisely were I've been putting in a ton of > work - making sure patches aren't left hanging. that's why i guess sending automatic mails would be a good way to remember that a reviewer had a patch in their control or to tell reviewers/committers there are still patches for review/commit -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Selena Deckelmann wrote:<br /><blockquote cite="mid:2b5e566d0911110812r40d5d91fu699cf5fac5ac5eda@mail.gmail.com" type="cite"><prewrap="">On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <a class="moz-txt-link-rfc2396E" href="mailto:gsmith@gregsmith.com"><gsmith@gregsmith.com></a>wrote: </pre><blockquote type="cite"><pre wrap="">I wasjust poking around on the Wiki, and it looks like the role of the CommitFest manager isn't very well documented yet. </pre></blockquote><pre wrap=""> It's pretty straightforward. Robert has actually done a great job of communicating about this to the patch reviewers. </pre></blockquote> That's good to hear. What I was hinting at was thatsome of the community knowledge here should start getting written down now that the process has matured, rather thantrying to directly transfer just to one other person. I'm not sure if Robert has shared 100% of what he does with thereviewers or not, but in general the easiest way to divest yourself of a position is to document how someone else cando it. I don't know that having to poke through list archives or chat with someone is necessarily the best way to transferthat knowledge.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
On Wed, Nov 11, 2009 at 5:06 PM, Jaime Casanova <jcasanov@systemguards.com.ec> wrote: > On Wed, Nov 11, 2009 at 4:40 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Nov 11, 2009 at 4:21 PM, Jaime Casanova >> <jcasanov@systemguards.com.ec> wrote: >>> why we need a full time manager at all? >>> why not simply use -rrreviewers to track the status of a patch? of >>> course, we hope the author or reviewer to change the status as >>> appropiate but we have seen many people including missing discussions >>> and changing the status of hanging patches... >> >> Well, actually, that's precisely were I've been putting in a ton of >> work - making sure patches aren't left hanging. > > that's why i guess sending automatic mails would be a good way to > remember that a reviewer had a patch in their control or to tell > reviewers/committers there are still patches for review/commit I think an automatic system would probably not be too valuable, but you're welcome to submit a patch against commitfest.postgresql.org (source code is published at git.postgresql.org). I'd recommend proposing a design on -hackers first. It's easy to generate systems that spew out a lot of email, but the system doesn't really have any understanding of what is really going on. When I send out emails to nag people, I actually put quite a bit of thought into what I say. Sometimes I try to summarize the current status of the patch, sometimes I add my own thoughts, sometimes I just fire off a one-liner. I think that adds value, but perhaps I overestimate myself. ...Robert
On Wed, Nov 11, 2009 at 5:14 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Selena Deckelmann wrote: > > On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith@gregsmith.com> wrote: > > > I was just poking around on the Wiki, and it looks like the role of the > CommitFest manager isn't very well documented yet. > > > It's pretty straightforward. Robert has actually done a great job of > communicating about this to the patch reviewers. > > > That's good to hear. What I was hinting at was that some of the community > knowledge here should start getting written down now that the process has > matured, rather than trying to directly transfer just to one other person. > I'm not sure if Robert has shared 100% of what he does with the reviewers or > not, but in general the easiest way to divest yourself of a position is to > document how someone else can do it. I don't know that having to poke > through list archives or chat with someone is necessarily the best way to > transfer that knowledge. I'll try to write something up. Stand by. ...Robert
Robert Haas escreveu: > I think an automatic system would probably not be too valuable > I have the same impression. > It's easy to generate systems that spew out a lot of email, but the > system doesn't really have any understanding of what is really going > on. When I send out emails to nag people, I actually put quite a bit > of thought into what I say. Sometimes I try to summarize the current > status of the patch, sometimes I add my own thoughts, sometimes I just > fire off a one-liner. I think that adds value, but perhaps I > overestimate myself. > But I think we should try to have multiple CMs so we don't burn out someone else. That group (a small one) could do the same job Robert has been done (AFAICS a very good job) but in a distributed way. I certainly could be a CM if I don't have to dedicate too much time at the CF month. I don't know if it would be as effective as it has been done but it will take that weight off our CM's shoulders. -- Euler Taveira de Oliveira http://www.timbira.com/
On Wed, Nov 11, 2009 at 5:27 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Nov 11, 2009 at 5:14 PM, Greg Smith <greg@2ndquadrant.com> wrote: >> Selena Deckelmann wrote: >> >> On Tue, Nov 10, 2009 at 10:40 PM, Greg Smith <gsmith@gregsmith.com> wrote: >> >> >> I was just poking around on the Wiki, and it looks like the role of the >> CommitFest manager isn't very well documented yet. >> >> >> It's pretty straightforward. Robert has actually done a great job of >> communicating about this to the patch reviewers. >> >> >> That's good to hear. What I was hinting at was that some of the community >> knowledge here should start getting written down now that the process has >> matured, rather than trying to directly transfer just to one other person. >> I'm not sure if Robert has shared 100% of what he does with the reviewers or >> not, but in general the easiest way to divest yourself of a position is to >> document how someone else can do it. I don't know that having to poke >> through list archives or chat with someone is necessarily the best way to >> transfer that knowledge. > > I'll try to write something up. Stand by. Here's an attempt. http://wiki.postgresql.org/wiki/Running_a_CommitFest ...Robert
Robert Haas wrote: > Here's an attempt. > http://wiki.postgresql.org/wiki/Running_a_CommitFest > Perfect, that's the sort of thing I was looking for the other day but couldn't find anywhere. I just made a pass through better wiki-fying that and linking it to the related pages in this area. Two things look to be true at the moment: 1) The call for reviewers is already running late and needs to start ASAP. 2) Some of the experienced helpers from the previous CFs, like Selena, should eventually be able to help, just everybody is busy during when the first round of action has to happen here. Given all that, I'm thinking that unless we get an enthusiastic volunteer by tomorrow, I'll kick off the call for reviewers myself and follow that through to initial patch assignments. I don't expect to have as much time as Robert put into the last couple of CommitFests after that, but this one looks smaller and with more familiar patches than those. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Sun, 2009-11-08 at 20:52 -0500, Robert Haas wrote: > I would personally prefer not to be involved in the management of the > next CommitFest. Having done all of the July CommitFest and a good > chunk of the September CommitFest, I am feeling a bit burned out. You did a grand job and everybody appreciates it. > I feel like Simon Riggs and > Fujii Masao really pulled out all the stops to get these ready in time > for the September CommitFest, and while I'm not in a hurry to break > the world, I think the sooner these can hit the tree, the better of > we'll be in terms of releasing 8.5. Sprinting is hard and we all need to rest afterwards. How about we just slow the pace down a little? Nobody wants you to quit, we just need to set a sustainable pace. Looking at the submissions so far, it seems you've done such a grand job at clearing the backlog that there are few patches in the next fest. -- Simon Riggs www.2ndQuadrant.com
On Thu, Nov 12, 2009 at 5:04 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Sun, 2009-11-08 at 20:52 -0500, Robert Haas wrote: > >> I would personally prefer not to be involved in the management of the >> next CommitFest. Having done all of the July CommitFest and a good >> chunk of the September CommitFest, I am feeling a bit burned out. > > You did a grand job and everybody appreciates it. Thanks. >> I feel like Simon Riggs and >> Fujii Masao really pulled out all the stops to get these ready in time >> for the September CommitFest, and while I'm not in a hurry to break >> the world, I think the sooner these can hit the tree, the better of >> we'll be in terms of releasing 8.5. > > Sprinting is hard and we all need to rest afterwards. > > How about we just slow the pace down a little? Nobody wants you to quit, > we just need to set a sustainable pace. I'm not sure exactly what point you're aiming at here, so I'll respond with a few thoughts that may or may not pertain. I think it would be really, really good if we could make this release come out on the schedule previously discussed. 8.4 slipped quite a bit for reasons that were, IMHO, quite preventable: and, worse, it's not clear that the slippage really bought us anything, because we still ended up with a bunch of embarassing bugs. Having said that, I'm not capable of single-handedly effecting an on-time release, and I don't particularly want to. In a community where people can disappear or change roles in the snap of a finger, it's bad to be relying on any one person to do too much. We need larger, more robust pools of committers, reviewers, commitfest managers, etc. Perhaps for next release we should consider spacing the CommitFests out a little more. I think one CommitFest every 2 months is a little too tight a schedule. As Peter and others have mentioned previously, it doesn't leave a lot of time to work on your own patches (behold the lack of any of my patches in this CommitFest). I think a CommitFest every 3 months would be too long, but maybe something in the middle. The trick is to navigate around major holidays while ending around the right time. Possibly the amount of time between CommitFests doesn't even need to be constant throughout the release cycle - maybe shorter at the beginning and longer towards the end. > Looking at the submissions so far, it seems you've done such a grand job > at clearing the backlog that there are few patches in the next fest. Thanks for your kind words. It does seem that most of the major patches we've seen so far landed last CommitFest, with the exception of HS and SR. That's not all me, of course - among other people, Tom did a tremendous amount of work - but I'm glad I was able to help move it along. ...Robert
Hi, On Thursday 12 November 2009 12:46:46 Robert Haas wrote: > Perhaps for next release we should consider spacing the CommitFests > out a little more. That may lead to quite a bit frustration on the contributor side though. It can be very frustrating to have no input for a even longer timeframe... Andres
On Thu, Nov 12, 2009 at 6:49 AM, Andres Freund <andres@anarazel.de> wrote: > On Thursday 12 November 2009 12:46:46 Robert Haas wrote: >> Perhaps for next release we should consider spacing the CommitFests >> out a little more. > That may lead to quite a bit frustration on the contributor side though. It > can be very frustrating to have no input for a even longer timeframe... Yep, that's the flip side. But you'll notice I was suggesting extending it by a pretty small amount. Anyway, it was just a thought. ...Robert
On Thu, 2009-11-12 at 06:46 -0500, Robert Haas wrote: > Having said that, > I'm not capable of single-handedly effecting an on-time release You're bloody good and the task needs to fit our capability anyway. So, yes, you are. > We need larger, more robust pools of > committers, reviewers, commitfest managers, etc. We're living in a desert. We just need to remember it. Plan hard, focus on the important and be real. Move at a smooth pace to save resources. Don't give up when the going gets tough, just rest up and then continue. Not a new idea, but I think we should require all patch submitters to do one review per submission. There needs to be a balance between time spent on review and time spent on dev. The only real way this happens in any community is by peer review. All patch submitters need to know that they must also take their turn as patch reviewers. If it is a hard rule, then patch *sponsors* would be forced to accept that they must *also* pay for review time. It is the sponsors that need to be forced to accept that reality, though we can only "get at them" through controlling developer behaviour. So, I propose that we simply ignore patches from developers until they have done sufficient review to be allowed to develop again. -- Simon Riggs www.2ndQuadrant.com
On Wed, Nov 11, 2009 at 2:03 PM, Josh Berkus <josh@agliodbs.com> wrote: >> I'd be happy to get together at some pre-appointed hour this weekend >> (Saturday / Sunday) to talk it over by phone / IRC. PDXPUG was already >> planning to get together to review some patches this Sunday from 3-5pm >> PST, so that is a convenient time for me. > > Aren't you running OpenSQL this weekend? Yes :) But we have lots of volunteers. It's an unconference, and I delegated. -selena -- http://chesnok.com/daily - me http://endpoint.com - work
On Thu, Nov 12, 2009 at 11:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > Not a new idea, but I think we should require all patch submitters to do > one review per submission. There needs to be a balance between time > spent on review and time spent on dev. The only real way this happens in > any community is by peer review. > > All patch submitters need to know that they must also take their turn as > patch reviewers. If it is a hard rule, then patch *sponsors* would be > forced to accept that they must *also* pay for review time. It is the > sponsors that need to be forced to accept that reality, though we can > only "get at them" through controlling developer behaviour. So, I > propose that we simply ignore patches from developers until they have > done sufficient review to be allowed to develop again. I agree. I would quibble only with the details. I think we should require patch authors to act as a reviewer for any CommitFest for which they have submitted patches. We don't need every patch author to review as many patches as they submit, because some people will review extras, and some non-patch-authors will review. If they review one patch each, that's probably more than enough. It's also easier for bookkeeping purposes. ...Robert
On Thu, Nov 12, 2009 at 8:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > Not a new idea, but I think we should require all patch submitters to do > one review per submission. There needs to be a balance between time > spent on review and time spent on dev. The only real way this happens in > any community is by peer review. I like this idea. Perhaps also if a person is reviewing a patch for the first time, we could make some effort to get a more experienced person paired up with them. Pairing up was really useful for the PDXPUG folks working on review. -- http://chesnok.com/daily - me http://endpoint.com - work
Simon Riggs escreveu: > So, I > propose that we simply ignore patches from developers until they have > done sufficient review to be allowed to develop again. > Is it really impolite for a first-contributor, no? -- Euler Taveira de Oliveira http://www.timbira.com/
> I like this idea. Perhaps also if a person is reviewing a patch for > the first time, we could make some effort to get a more experienced > person paired up with them. When I was CFM last year, I assigned patches for review first if the patch author was doing a review themselves. Patches whose authors were not doing reviews often got reviewed last, which means that sometimes they waited a commitfest. Of course, Simon submitted so many patches that despite him doing reviews, it was hard to get them all assigned. Simon: slow down a little! ;-) --Josh Berkus
On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny <albert@nan-tic.com> wrote: > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure: >> Simon Riggs escreveu: >> > So, I >> > propose that we simply ignore patches from developers until they have >> > done sufficient review to be allowed to develop again. >> >> Is it really impolite for a first-contributor, no? >> > > I don't think so, as long as it's properly explained. Personally, I would not propose to impose this rule of first-time contributors, or even second-time contributors. But by about patch #3 I think everyone should be pitching in. Just MHO, of course. ...Robert
A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure: > Simon Riggs escreveu: > > So, I > > propose that we simply ignore patches from developers until they have > > done sufficient review to be allowed to develop again. > > Is it really impolite for a first-contributor, no? > I don't think so, as long as it's properly explained. -- Albert Cervera i Areny http://www.NaN-tic.com Mòbil: +34 669 40 40 18
2009/11/13 Euler Taveira de Oliveira <euler@timbira.com>: > Simon Riggs escreveu: >> So, I >> propose that we simply ignore patches from developers until they have >> done sufficient review to be allowed to develop again. >> > Is it really impolite for a first-contributor, no? > I support Simon's proposal, but I think we would certainly need to make an exception for first-time contributors. Cheers, BJ
On Thu, 2009-11-12 at 15:52 -0200, Euler Taveira de Oliveira wrote: > Simon Riggs escreveu: > > So, I > > propose that we simply ignore patches from developers until they have > > done sufficient review to be allowed to develop again. > > Is it really impolite for a first-contributor, no? I believe the community has always given extra help to first-time contributors. If anyone sees a new contributor being ignored, they should make an effort to help. Regards,Jeff Davis
Robert Haas escribió: > On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny > <albert@nan-tic.com> wrote: > > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure: > >> Simon Riggs escreveu: > >> > So, I > >> > propose that we simply ignore patches from developers until they have > >> > done sufficient review to be allowed to develop again. > >> > >> Is it really impolite for a first-contributor, no? > > > > I don't think so, as long as it's properly explained. > > Personally, I would not propose to impose this rule of first-time > contributors, or even second-time contributors. But by about patch #3 > I think everyone should be pitching in. +1. It's OK to not review anything if you're just doing a one-off. If you submit a bunch of patches you start becoming part of the community. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Thu, 2009-11-12 at 11:36 -0500, Robert Haas wrote: > I agree. I would quibble only with the details. I think we should > require patch authors to act as a reviewer for any CommitFest for > which they have submitted patches. We don't need every patch author > to review as many patches as they submit, because some people will > review extras, and some non-patch-authors will review. If they review > one patch each, that's probably more than enough. It's also easier > for bookkeeping purposes. Not all contributors are fluent English readers and writers. I certainly do not discourage those people from reviewing, but I can understand how it might be more frustrating and less productive for them. An important part of review is to read the relevant mailing list threads and try to tie up loose ends and find a consensus. Regards,Jeff Davis
On Thu, Nov 12, 2009 at 1:45 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Thu, 2009-11-12 at 11:36 -0500, Robert Haas wrote: >> I agree. I would quibble only with the details. I think we should >> require patch authors to act as a reviewer for any CommitFest for >> which they have submitted patches. We don't need every patch author >> to review as many patches as they submit, because some people will >> review extras, and some non-patch-authors will review. If they review >> one patch each, that's probably more than enough. It's also easier >> for bookkeeping purposes. > > Not all contributors are fluent English readers and writers. > > I certainly do not discourage those people from reviewing, but I can > understand how it might be more frustrating and less productive for > them. An important part of review is to read the relevant mailing list > threads and try to tie up loose ends and find a consensus. Unfortunately, those people's patches also typically require more work from other community members. Discussions are longer and more confused, and someone has to rewrite the documentation and comments before commit. If anything, people whose patches require more help need to find ways to contribute MORE to the community, not less. I understand that's not easy, but it's necessary. ...Robert
On Thu, 2009-11-12 at 14:43 -0500, Robert Haas wrote: > > Not all contributors are fluent English readers and writers. > > > > I certainly do not discourage those people from reviewing, but I can > > understand how it might be more frustrating and less productive for > > them. An important part of review is to read the relevant mailing list > > threads and try to tie up loose ends and find a consensus. > > Unfortunately, those people's patches also typically require more work > from other community members. Discussions are longer and more > confused, and someone has to rewrite the documentation and comments > before commit. If anything, people whose patches require more help > need to find ways to contribute MORE to the community, not less. I > understand that's not easy, but it's necessary. Keep in mind that many of these people may also be regional contacts, translators, local conference (or user group) organizers, and provide support to users in languages other than English. They may even organize development efforts in languages other than English. I think the best policies encourage people to help in the ways that they are most effective, and/or enjoy the most. I know that's a general statement, and it doesn't translate (excuse the pun) easily into policy. I don't have a good answer for that. But sometimes the policy discussions here seem a little abstract, and I have a hard time seeing how they apply in real situations. It seems like you have a few "freeloaders" in mind, but I have a hard time imagining it's more than a couple people. Maybe they just need a nice reminder to be a more helpful community member if they want timely feedback on their work? Regards,Jeff Davis
On Thu, Nov 12, 2009 at 3:12 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Thu, 2009-11-12 at 14:43 -0500, Robert Haas wrote: >> > Not all contributors are fluent English readers and writers. >> > >> > I certainly do not discourage those people from reviewing, but I can >> > understand how it might be more frustrating and less productive for >> > them. An important part of review is to read the relevant mailing list >> > threads and try to tie up loose ends and find a consensus. >> >> Unfortunately, those people's patches also typically require more work >> from other community members. Discussions are longer and more >> confused, and someone has to rewrite the documentation and comments >> before commit. If anything, people whose patches require more help >> need to find ways to contribute MORE to the community, not less. I >> understand that's not easy, but it's necessary. > > Keep in mind that many of these people may also be regional contacts, > translators, local conference (or user group) organizers, and provide > support to users in languages other than English. They may even organize > development efforts in languages other than English. They may, but it's beyond my power to keep track of everything that a person may or may not do. If we're going to have any chance of enforcing a policy, it has to be simple and clear. > I don't have a good answer for that. But sometimes the policy > discussions here seem a little abstract, and I have a hard time seeing > how they apply in real situations. It seems like you have a few > "freeloaders" in mind, but I have a hard time imagining it's more than a > couple people. Maybe they just need a nice reminder to be a more helpful > community member if they want timely feedback on their work? Well, you can go back and look at the list of people who had patches reviewed last CF (see commitfest.postgresql.org), and then go back and look at the reviewers (see pgsql-rrreviewers archives). It's all public data. It's not 1 or 2 people - I count at least 8. I could cross-reference for you and list the names, but that seems like embarassing those people without accomplishing anything useful. ...Robert
Robert Haas wrote: > On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny > <albert@nan-tic.com> wrote: > > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure: > >> Simon Riggs escreveu: > >> > So, I > >> > propose that we simply ignore patches from developers until they have > >> > done sufficient review to be allowed to develop again. > >> > >> Is it really impolite for a first-contributor, no? > >> > > > > I don't think so, as long as it's properly explained. > > Personally, I would not propose to impose this rule of first-time > contributors, or even second-time contributors. But by about patch #3 > I think everyone should be pitching in. I hate to ask, but how would we enforce this? Do we no longer apply patches for 3rd-time submitters who have not reviewed? That seems to be hurting us more than them. Are we prepared to discard valid patches for this reason? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, Nov 12, 2009 at 9:15 PM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> On Thu, Nov 12, 2009 at 1:25 PM, Albert Cervera i Areny >> <albert@nan-tic.com> wrote: >> > A Dijous, 12 de novembre de 2009, Euler Taveira de Oliveira va escriure: >> >> Simon Riggs escreveu: >> >> > So, I >> >> > propose that we simply ignore patches from developers until they have >> >> > done sufficient review to be allowed to develop again. >> >> >> >> Is it really impolite for a first-contributor, no? >> >> >> > >> > I don't think so, as long as it's properly explained. >> >> Personally, I would not propose to impose this rule of first-time >> contributors, or even second-time contributors. But by about patch #3 >> I think everyone should be pitching in. > > I hate to ask, but how would we enforce this? Do we no longer apply > patches for 3rd-time submitters who have not reviewed? That seems to be > hurting us more than them. Are we prepared to discard valid patches > for this reason? We just wouldn't assign round-robin reviewers to such patches. If someone wants to volunteer, more power to them, but we would encourage people to focus their efforts on the patches of people who were themselves reviewing. It's important to keep in mind that "valid" is not a boolean. Some patches are perfect the day they roll in, but not too many. It takes work to get them committable, and I don't see why anyone should have an expectation that they can have that help for themselves without doing the same thing for other people. All that having been said, the real shortage ATM is of committers rather than reviewers. We have plenty of them, but many of them commit almost nothing. I don't want to minimize the contributions of the non-Tom committers, but Tom is numerically far and away committing more than anyone else, and not small patches, either. Beyond the fact that it makes the CommitFest slow, long, and not too much fun for Tom, it also means that Tom has less time available to do things that Only Tom Can Do. I venture to say that there will be Great Excitement about the enhancements to the EPQ machinery and PL/pgsql that Tom has recently effected. Well, if Tom hadn't had to single-handedly handle so many patches last CF, maybe he would have done something else cool, too. ...Robert
Robert Haas wrote: > >> Personally, I would not propose to impose this rule of first-time > >> contributors, or even second-time contributors. ?But by about patch #3 > >> I think everyone should be pitching in. > > > > I hate to ask, but how would we enforce this? ?Do we no longer apply > > patches for 3rd-time submitters who have not reviewed? ?That seems to be > > hurting us more than them. ? Are we prepared to discard valid patches > > for this reason? > > We just wouldn't assign round-robin reviewers to such patches. If > someone wants to volunteer, more power to them, but we would encourage > people to focus their efforts on the patches of people who were > themselves reviewing. It's important to keep in mind that "valid" is > not a boolean. Some patches are perfect the day they roll in, but not > too many. It takes work to get them committable, and I don't see why > anyone should have an expectation that they can have that help for > themselves without doing the same thing for other people. OK, but the problem I see there is that the reviewers are there to assist the committers; if no one reviews something, it just makes more work for the committers. > All that having been said, the real shortage ATM is of committers > rather than reviewers. We have plenty of them, but many of them > commit almost nothing. I don't want to minimize the contributions of > the non-Tom committers, but Tom is numerically far and away committing > more than anyone else, and not small patches, either. Beyond the > fact that it makes the CommitFest slow, long, and not too much fun for > Tom, it also means that Tom has less time available to do things that > Only Tom Can Do. I venture to say that there will be Great Excitement > about the enhancements to the EPQ machinery and PL/pgsql that Tom has > recently effected. Well, if Tom hadn't had to single-handedly handle > so many patches last CF, maybe he would have done something else cool, > too. Totally agree. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, Nov 12, 2009 at 11:31 PM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> >> Personally, I would not propose to impose this rule of first-time >> >> contributors, or even second-time contributors. ?But by about patch #3 >> >> I think everyone should be pitching in. >> > >> > I hate to ask, but how would we enforce this? ?Do we no longer apply >> > patches for 3rd-time submitters who have not reviewed? ?That seems to be >> > hurting us more than them. ? Are we prepared to discard valid patches >> > for this reason? >> >> We just wouldn't assign round-robin reviewers to such patches. If >> someone wants to volunteer, more power to them, but we would encourage >> people to focus their efforts on the patches of people who were >> themselves reviewing. It's important to keep in mind that "valid" is >> not a boolean. Some patches are perfect the day they roll in, but not >> too many. It takes work to get them committable, and I don't see why >> anyone should have an expectation that they can have that help for >> themselves without doing the same thing for other people. > > OK, but the problem I see there is that the reviewers are there to > assist the committers; if no one reviews something, it just makes more > work for the committers. That wasn't my intention. I really was assuming that we would just let those patches drop on the floor, and that they would not be picked up either by reviewers or committers. I don't think this would cause as many problems in practice as perhaps you fear, because I think it will just motivate people to act as reviewers. Writing a patch is typically more time-consuming than reviewing one, at least IME, with some exceptions of course. I wouldn't spend 20 hours writing a patch and then let it fall out because I wasn't willing to spend 2 or 3 hours reviewing someone else's patch, and I don't think other regular contributors will either. ...Robert
Robert Haas wrote: > >> We just wouldn't assign round-robin reviewers to such patches. ?If > >> someone wants to volunteer, more power to them, but we would encourage > >> people to focus their efforts on the patches of people who were > >> themselves reviewing. ?It's important to keep in mind that "valid" is > >> not a boolean. ?Some patches are perfect the day they roll in, but not > >> too many. ?It takes work to get them committable, and I don't see why > >> anyone should have an expectation that they can have that help for > >> themselves without doing the same thing for other people. > > > > OK, but the problem I see there is that the reviewers are there to > > assist the committers; ?if no one reviews something, it just makes more > > work for the committers. > > That wasn't my intention. I really was assuming that we would just > let those patches drop on the floor, and that they would not be picked > up either by reviewers or committers. I don't think this would cause > as many problems in practice as perhaps you fear, because I think it > will just motivate people to act as reviewers. Writing a patch is > typically more time-consuming than reviewing one, at least IME, with > some exceptions of course. I wouldn't spend 20 hours writing a patch > and then let it fall out because I wasn't willing to spend 2 or 3 > hours reviewing someone else's patch, and I don't think other regular > contributors will either. OK, but that is certainly a different system than we have now. In your system, committers would be told to ignore patches that were submitted by repeated patch submitters who never review, or even we just never put on the commit fest page. I am just trying to nail down exactly how that would work --- that's a pretty Draconian system. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, Nov 12, 2009 at 11:50 PM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> >> We just wouldn't assign round-robin reviewers to such patches. ?If >> >> someone wants to volunteer, more power to them, but we would encourage >> >> people to focus their efforts on the patches of people who were >> >> themselves reviewing. ?It's important to keep in mind that "valid" is >> >> not a boolean. ?Some patches are perfect the day they roll in, but not >> >> too many. ?It takes work to get them committable, and I don't see why >> >> anyone should have an expectation that they can have that help for >> >> themselves without doing the same thing for other people. >> > >> > OK, but the problem I see there is that the reviewers are there to >> > assist the committers; ?if no one reviews something, it just makes more >> > work for the committers. >> >> That wasn't my intention. I really was assuming that we would just >> let those patches drop on the floor, and that they would not be picked >> up either by reviewers or committers. I don't think this would cause >> as many problems in practice as perhaps you fear, because I think it >> will just motivate people to act as reviewers. Writing a patch is >> typically more time-consuming than reviewing one, at least IME, with >> some exceptions of course. I wouldn't spend 20 hours writing a patch >> and then let it fall out because I wasn't willing to spend 2 or 3 >> hours reviewing someone else's patch, and I don't think other regular >> contributors will either. > > OK, but that is certainly a different system than we have now. In your > system, committers would be told to ignore patches that were submitted > by repeated patch submitters who never review, or even we just never put > on the commit fest page. I think they would probably get added to the CommitFest page and then marked Rejected with a suitable explanation. > I am just trying to nail down exactly how that would work --- that's a > pretty Draconian system. I don't really agree, but obviously I respect your opinion, and clearly, this is not a policy that can be implemented without some degree of consensus. I fear, however, that if we don't motivate regular contributors to also review, then we will have a shortage of reviewers, especially highly-qualified reviewers. If there is no stigma attached to submitting patches and never volunteering to review, then even people who have reviewed in the past may eventually decide it isn't worth the effort. I am personally quite tired of reviewing patches for people who don't in turn review mine (or someone's). It makes me feel like not working on this project. If we can solve that problem without implementing a policy of this type, that is good. I would much prefer to run by the honor system rather than having to threaten to drop patches, but only if the honor system actually works. ...Robert
Robert Haas wrote: > > That wasn't my intention. I really was assuming that we would just > let those patches drop on the floor, and that they would not be picked > up either by reviewers or committers. Surely it should depend on the nature of the patch. For an extreme strawman, segfault fixes almost certainly shouldn't be dropped. Same for docs patches that clarify the product. I think the majority of my contributions to open source this decade have been of that nature (a few links to examples in postgres and postgis follow). Maybe a better policy would be: "if you reviewed patches, a reviewer will be assigned -- if you didn't, your patch is atthe mercy of reviewers volunteering to review it based on their own interest in your patch" that way patches that the community really wants could get in anyway. http://postgis.refractions.net/pipermail/postgis-users/2005-April/007762.html http://archives.postgresql.org/pgsql-performance/2009-03/msg00252.php http://postgis.refractions.net/pipermail/postgis-users/2005-April/007704.html http://postgis.refractions.net/pipermail/postgis-devel/2005-April/001341.html
On Fri, Nov 13, 2009 at 2:15 AM, Bruce Momjian <bruce@momjian.us> wrote: >> Personally, I would not propose to impose this rule of first-time >> contributors, or even second-time contributors. But by about patch #3 >> I think everyone should be pitching in. > > I hate to ask, but how would we enforce this? Do we no longer apply > patches for 3rd-time submitters who have not reviewed? That seems to be > hurting us more than them. Are we prepared to discard valid patches > for this reason? What about people who contribute hours and hours of their time in other ways? Are they required to contribute even more of their time to review as well, just to help their own occasional code contributions get through the process? (yes, I am thinking largely of me, working tens of hours per week on Postgres, but perhaps submiting one relatively small patch per release) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Fri, 2009-11-13 at 08:33 +0000, Dave Page wrote: > On Fri, Nov 13, 2009 at 2:15 AM, Bruce Momjian <bruce@momjian.us> wrote: > > >> Personally, I would not propose to impose this rule of first-time > >> contributors, or even second-time contributors. But by about patch #3 > >> I think everyone should be pitching in. > > > > I hate to ask, but how would we enforce this? Do we no longer apply > > patches for 3rd-time submitters who have not reviewed? That seems to be > > hurting us more than them. Are we prepared to discard valid patches > > for this reason? > > What about people who contribute hours and hours of their time in > other ways? Are they required to contribute even more of their time to > review as well, just to help their own occasional code contributions > get through the process? Yes, but the rule needs to be hard for a few reasons. If your employer encourages you to write a patch then they need to be made aware that you must also do all the other things too: write docs, supply tests, explain it *and* include review time. Since reviewing your own patch isn't possible, we must require patch review of other's patches. The rule is deliberately hard on the contributor to ensure the sponsor understands the rule and allows more time. I don't expect it to need to be enforced. If everybody understands it will be enforced and that there will be some polite reminders, then it will likely never need to be enforced. For me, reviewing patches is a great way to learn how to code properly, learn PostgreSQL and get new ideas. All the CF manager needs to do is ensure that every patch submitted chalks up one review. If you think about it, we wouldn't actually need any rr reviewers at all then, because if we have 20 patches we would have 20 reviews due. So the whole scheme is self-balancing. -- Simon Riggs www.2ndQuadrant.com
On Fri, Nov 13, 2009 at 8:50 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> What about people who contribute hours and hours of their time in >> other ways? Are they required to contribute even more of their time to >> review as well, just to help their own occasional code contributions >> get through the process? > > Yes, but the rule needs to be hard for a few reasons. If your employer > encourages you to write a patch then they need to be made aware that you > must also do all the other things too: write docs, supply tests, explain > it *and* include review time. Since reviewing your own patch isn't > possible, we must require patch review of other's patches. The rule is > deliberately hard on the contributor to ensure the sponsor understands > the rule and allows more time. Which is fine for sponsored patches, but my employer doesn't sponsor me to hack on PostgreSQL directly - we have other staff that are far more capable of doing that :-). My annual submissions are for my own personal development/enjoyment/whatever, and are never chosen for my employer, but because they seem interesting, are common requests from users, or would help pgAdmin provide improved functionality for users. The bottom line is, if I'm required to review patches, it will impact on other community work, which I am almost certainly more qualified to do and would be more productive at. That said - in general I don't disagree with the idea of everyone chipping in, and I do try to do so myself on appropriate patches (for example, the recent Windows DACL bug fix which I spent a few hours reviewing and testing - and am still waiting for Magnus to commit :-p). I just think that *requiring* people to review will ultimately be counter productive. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Fri, 2009-11-13 at 10:26 +0000, Dave Page wrote: > On Fri, Nov 13, 2009 at 8:50 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > >> What about people who contribute hours and hours of their time in > >> other ways? Are they required to contribute even more of their time to > >> review as well, just to help their own occasional code contributions > >> get through the process? > > > > Yes, but the rule needs to be hard for a few reasons. If your employer > > encourages you to write a patch then they need to be made aware that you > > must also do all the other things too: write docs, supply tests, explain > > it *and* include review time. Since reviewing your own patch isn't > > possible, we must require patch review of other's patches. The rule is > > deliberately hard on the contributor to ensure the sponsor understands > > the rule and allows more time. > > Which is fine for sponsored patches, but my employer doesn't sponsor > me to hack on PostgreSQL directly - we have other staff that are far > more capable of doing that :-). My annual submissions are for my own > personal development/enjoyment/whatever, and are never chosen for my > employer, but because they seem interesting, are common requests from > users, or would help pgAdmin provide improved functionality for users. > > The bottom line is, if I'm required to review patches, it will impact > on other community work, which I am almost certainly more qualified to > do and would be more productive at. > > That said - in general I don't disagree with the idea of everyone > chipping in, and I do try to do so myself on appropriate patches (for > example, the recent Windows DACL bug fix which I spent a few hours > reviewing and testing - and am still waiting for Magnus to commit > :-p). I just think that *requiring* people to review will ultimately > be counter productive. Requiring people to write docs or any other patch submission rules has never been counterproductive. People could easily say, "English is not my first language, therefore I skip all comments and docs". But they don't, because we require that, as a hard rule. Nobody has ever said enforcing *those* rules is counter productive. I don't see why adding new requirements would be a problem - especially since they aim to address problems with the flow of patches. Change will always seem strange, but just like commitfests themselves the new way of working has been quickly adopted without much complaint. Bottom line is if we all spend all of our time developing and no time reviewing, then we shouldn't be surprised if there is a review bottleneck. Everybody wants their patches to go through, and the *fastest* way is actually for people to assist with review. We just need an easily understood way of implementing that. The 1:1 suggestion is one way, there may be others. -- Simon Riggs www.2ndQuadrant.com
On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > Requiring people to write docs or any other patch submission rules has > never been counterproductive. People could easily say, "English is not > my first language, therefore I skip all comments and docs". But they > don't, because we require that, as a hard rule. Nobody has ever said > enforcing *those* rules is counter productive. Requiring that someone document their own work is very different from requiring that they spend time reviewing someone elses entirely unrelated work, possibly in areas of which they have little or no understanding (which may well be an issue at times). -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Robert Haas wrote: > I am personally quite tired of > reviewing patches for people who don't in turn review mine (or > someone's). It makes me feel like not working on this project. If we > can solve that problem without implementing a policy of this type, > that is good. I would much prefer to run by the honor system rather > than having to threaten to drop patches, but only if the honor system > actually works. > > > Organizing contributors on a project like this is like herding cats. Threats and penalties are unlikely to be effective. This is essentially a charity where people give in ways that work for them, and you take whatever they have to give. I'm extremely uncomfortable with the idea of a prescriptive system. I've proposed them myself in the past, but I have since come to the realization that it will simply drive people away. cheers andrew
Simon Riggs wrote: > Requiring people to write docs or any other patch submission rules has > never been counterproductive. People could easily say, "English is not > my first language, therefore I skip all comments and docs". But they > don't, because we require that, as a hard rule. Nobody has ever said > enforcing *those* rules is counter productive. I don't see why adding > new requirements would be a problem - especially since they aim to > address problems with the flow of patches. Change will always seem > strange, but just like commitfests themselves the new way of working has > been quickly adopted without much complaint. Bottom line is if we all > spend all of our time developing and no time reviewing, then we > shouldn't be surprised if there is a review bottleneck. Everybody wants > their patches to go through, and the *fastest* way is actually for > people to assist with review. We just need an easily understood way of > implementing that. The 1:1 suggestion is one way, there may be others. The docs case is a good example. We do ask people to write docs, but I don't think we will reject patches if people don't supply docs. I am not against any of the ideas suggested in this thread --- I am just pointing out we are heading in a very new direction with the _requirements_ mentioned. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Fri, 2009-11-13 at 13:34 +0000, Dave Page wrote: > On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > > Requiring people to write docs or any other patch submission rules has > > never been counterproductive. People could easily say, "English is not > > my first language, therefore I skip all comments and docs". But they > > don't, because we require that, as a hard rule. Nobody has ever said > > enforcing *those* rules is counter productive. > > Requiring that someone document their own work is very different from > requiring that they spend time reviewing someone elses entirely > unrelated work, possibly in areas of which they have little or no > understanding (which may well be an issue at times). Of course: one requirement is for docs, the other for review. OTOH they are both additional requirements around submitting a patch. Once people accept that, it will all work. All patches require review. If we have no mechanism for providing review time, then *all* patches will stall. I think it is unfair and unwise to assume that reviewers just turn up as needed. The reason we are having this discussion is they plainly don't. We were worried about Tom getting burnt out by it, now Robert is. I've no problem with arguing against my specific idea for producing more review time, but if there is no alternative proposal then all you are saying is "lets not fix the current problem". -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-13 at 08:46 -0500, Andrew Dunstan wrote: > Organizing contributors on a project like this is like herding cats. > Threats and penalties are unlikely to be effective. People have spoken against this because of the enforcement issue. If we talk about this like we were suggesting hands be removed, then sure, everybody will be against it. If we enforce it like we do other rules, such as "if your patch is late, we bump to next commit fest". That is "enforced in a draconian manner" but nobody thinks that is bad. If you imagine the suggestion with a softer focus, where we each act in a way that respects the need for an action that needs to happen, then it will work. -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-13 at 08:47 -0500, Bruce Momjian wrote: > We do ask people to write docs, but I > don't think we will reject patches if people don't supply docs. Yes, that is a good example. It's "a rule", plain and simple. Nobody gets their spleen removed for breaking it, yet it is still somehow enforced. I find it strange that suggesting a new rule is opposed on the general basis that *any* rule cannot be enforced; surely therefore we cannot have new rules at all, ever? We clearly do have new rules from time to time. So what's wrong with this new rule? Should we update the FAQ to say, "enclosing docs with a patch is a rule, but actually its not really and you only suffer mild rebuke if you break it and can therefore be ignored"? -- Simon Riggs www.2ndQuadrant.com
Simon Riggs wrote: > On Fri, 2009-11-13 at 08:47 -0500, Bruce Momjian wrote: > > > We do ask people to write docs, but I > > don't think we will reject patches if people don't supply docs. > > Yes, that is a good example. It's "a rule", plain and simple. Nobody > gets their spleen removed for breaking it, yet it is still somehow > enforced. > > I find it strange that suggesting a new rule is opposed on the general > basis that *any* rule cannot be enforced; surely therefore we cannot > have new rules at all, ever? We clearly do have new rules from time to > time. So what's wrong with this new rule? > > Should we update the FAQ to say, "enclosing docs with a patch is a rule, > but actually its not really and you only suffer mild rebuke if you break > it and can therefore be ignored"? Well, right now we ask for docs, but if they are not supplied, I think we just write them ourselves. Is a different enforcement method being suggested here? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Fri, 2009-11-13 at 09:31 -0500, Bruce Momjian wrote: > Well, right now we ask for docs, but if they are not supplied, I think > we just write them ourselves. Is a different enforcement method being > suggested here? And we never bump late patches, nor reject them if sent in missing format etc. We enforce all manner of rules. You tell me this is your main function on the project even. Is there now no enforcement of any rule, just because I propose a new one? Address the main question: how will we get more review time? There are many other possible proposals, so please lets hear them. (And how would they be enforced?) -- Simon Riggs www.2ndQuadrant.com
On Fri, Nov 13, 2009 at 8:47 AM, Bruce Momjian <bruce@momjian.us> wrote: > The docs case is a good example. We do ask people to write docs, but I > don't think we will reject patches if people don't supply docs. I am > not against any of the ideas suggested in this thread --- I am just > pointing out we are heading in a very new direction with the > _requirements_ mentioned. We reject patches for lack of docs all the time. We certainly don't have a policy that the reviewer or committer will write the docs for you if you fail to write them yourself. Sometimes the reviewer or committer will help copy edit, or will revise, but in most cases they won't write them from scratch. Of course, we don't reject such patches PERMANENTLY - people just add the docs and resubmit. ...Robert
Robert Haas wrote: > On Fri, Nov 13, 2009 at 8:47 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> The docs case is a good example. We do ask people to write docs, but I >> don't think we will reject patches if people don't supply docs. I am >> not against any of the ideas suggested in this thread --- I am just >> pointing out we are heading in a very new direction with the >> _requirements_ mentioned. >> > > We reject patches for lack of docs all the time. We certainly don't > have a policy that the reviewer or committer will write the docs for > you if you fail to write them yourself. Sometimes the reviewer or > committer will help copy edit, or will revise, but in most cases they > won't write them from scratch. > > Of course, we don't reject such patches PERMANENTLY - people just add > the docs and resubmit. > > > In that case people are working on their own patches. That's quite different from asking/requiring them to work on somebody else's. cheers andrew
On Fri, Nov 13, 2009 at 8:46 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > Robert Haas wrote: >> >> I am personally quite tired of >> reviewing patches for people who don't in turn review mine (or >> someone's). It makes me feel like not working on this project. If we >> can solve that problem without implementing a policy of this type, >> that is good. I would much prefer to run by the honor system rather >> than having to threaten to drop patches, but only if the honor system >> actually works. > > Organizing contributors on a project like this is like herding cats. Threats > and penalties are unlikely to be effective. This is essentially a charity > where people give in ways that work for them, and you take whatever they > have to give. I'm extremely uncomfortable with the idea of a prescriptive > system. I've proposed them myself in the past, but I have since come to the > realization that it will simply drive people away. I think you're entirely missing the point. If you want to spend large amounts of your time reviewing patches from people who won't in turn review yours, that is *absolutely wonderful*. It would be a huge help to the project, especially because you have the ability not only to review them, but also commit them, an area in which we are hugely strapped for qualified people who have the ability to commit substantial time to the project. I would be more than happy to cut back on managing CommitFests and reviewing patches and focus on writing my own patches and letting you fix them up and commit them. I have several good, interesting ideas for really cool patches that I don't have time to write, and they are posted on the developer Wiki if you'd like to go read them. Personally, I believe that I take more of an interest in other people's patches than the average contributor. I am interested in reviewing them, and I would be willing to put even more work in exchange for a commit bit, but that offer has not yet been forthcoming; maybe some day. I have done a large percentage of the management work for both of the last two CommitFests, during which time I have also reviewed 8 patches. That work was long, hard, difficult, and time-consuming. I wish to get something back for it, and specifically what I want to get back for it is the willingness of other contributors to review my patches when and if I can get enough of a break from running CommitFests and reviewing patches to write them. I don't find that setting that expectation is either unreasonable or unfair, and I'm sorry that you and Bruce apparently feel otherwise. What I would like to know is - assuming you don't want to do it yourself, and you don't want to require other *regular contributors* to do it - then who is going to review MY patches? Keep in mind that this is a problem that *does not apply to you*. You are a committer. If no one reviews your patch, you will eventually go ahead and commit it anyway. If no one reviews my patch, it doesn't go in. In fact, even if someone DOES review it, it doesn't necessarily go in, but at least the odds are better. Please don't sabotage my effort to ensure an adequate supply of reviewers unless you have a competing proposal. ...Robert
* Andrew Dunstan <andrew@dunslane.net> [091113 09:52]: > In that case people are working on their own patches. That's quite > different from asking/requiring them to work on somebody else's. But is it? Just s/patches/itches/ i.e. The "patched code implenting feature $X" is their main itch... They scratch that, and massage some other places (like docs and tests) to make their itch more palatable/pleasant for the rest of the community, because the community expects docs, tests, and reviews. a. -- Aidan Van Dyk Create like a god, aidan@highrise.ca command like a king, http://www.highrise.ca/ work like a slave.
On Fri, Nov 13, 2009 at 8:34 AM, Dave Page <dpage@pgadmin.org> wrote: > On Fri, Nov 13, 2009 at 1:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > >> Requiring people to write docs or any other patch submission rules has >> never been counterproductive. People could easily say, "English is not >> my first language, therefore I skip all comments and docs". But they >> don't, because we require that, as a hard rule. Nobody has ever said >> enforcing *those* rules is counter productive. > > Requiring that someone document their own work is very different from > requiring that they spend time reviewing someone elses entirely > unrelated work, possibly in areas of which they have little or no > understanding (which may well be an issue at times). I think this is overstating the level of contribution that is being asked, at least by me. Every CommitFest is full of a bunch of little patches that make small changes and need to be reviewed. Anyone who is reasonably familiar with C (and most of our regular contributors are) can take a little time to understand what one of those patches is doing and check it for style and functionality. I can usually review one of those patches in about 2 hours knowing nothing about that area of the system. What I object to about the present system is that the last two CommitFests have gone like this, for me: 1. First, I assign reviewers to as many patches as we have reviewers for. 2. Then, I ask the reviewers who haven't done so to actually complete the reviews. 3. Then, I ask the patch submitters who haven't done so to update their patches, and/or bounce the patches for lack of an update. 4. Then, I ask the reviewers who don't do it without prompting to check over the revised patches. ...at this point we are about 2 weeks into the CommitFest... 5. Then, I try to find reviewers for the patches that haven't been reviewed yet and start steps 1-4 over with the remaining patches. 6. Concurrently, I review several patches myself. Step #5 is the one that is really irritating to me. If there are 45 patches in the CommitFest and 15 to 20 people who didn't submit anything sign up to review and the committers take 7 or 8 patches directly, why is the number of reviewers still 10 or 15 less than the number of patches? My diagnosis is that the people who submit 2 or 3 or 4 or 5 patches to the CommitFest think to themselves "well, if I sign up to review, I might not have time to update all of these patches and get them committed during this CommitFest, so I'll let someone else do it". As far as I can see, this is the exact opposite of how the process is supposed to work: during the CommitFest, people are supposed to stop working on their own patches and review patches belonging to other people. As it is, the people who are actually willing to review are getting asked to review multiple patches so that other people can review none at all. Some people are willing to do that, and that is fine, but many are not, and even for the ones who are, after a certain point, it seems unfair to ask it. To put this another way, if everyone who submitted a patch reviewed a patch, we could finish up each CommitFest in 2-3 weeks instead of a whole month, except that the committing would drag out for the rest of the month unless someone other than Tom is willing to help to a greater degree than in the most recent CommitFest. That would substantially increase the time available for everyone to work on their own patches. ...Robert
Simon Riggs <simon@2ndQuadrant.com> writes:
> All the CF manager needs to do is ensure that every patch submitted
> chalks up one review. If you think about it, we wouldn't actually need
> any rr reviewers at all then, because if we have 20 patches we would
> have 20 reviews due. So the whole scheme is self-balancing.
Well, no, that's *far* too optimistic/simplistic, because it imagines
that every review is worth the same. What we lack is not just review
time but qualified review time, ie, comments from someone who's already
familiar with the portion of the code base that's being patched.
Now when the current reviewing process was proposed, there were two
separate goals in mind. One was to take whatever incremental load
we could off the eventual committer's work, by catching obvious
mistakes, making sure the docs were up to snuff, etc. The other was
the idea that reviewing would in itself improve the skills of our
development community, by making people read code that they wouldn't
have read otherwise, and that eventually we'd have more committer-grade
people just because of all the reviewing they'd done. (The jury is
still out on whether that will work, but in any case it's a long-term
project.)
The problem at the moment seems to not be lack of first-level review
time but lack of qualified review. I don't know what we do about that.
Requiring people who have submitted one or two patches to do reviews
isn't going to produce more of the latter, it's going to produce more
of the former. Especially if the patches available to be reviewed
are working in areas they haven't looked at before.
regards, tom lane
On Fri, Nov 13, 2009 at 10:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> All the CF manager needs to do is ensure that every patch submitted >> chalks up one review. If you think about it, we wouldn't actually need >> any rr reviewers at all then, because if we have 20 patches we would >> have 20 reviews due. So the whole scheme is self-balancing. > > Well, no, that's *far* too optimistic/simplistic, because it imagines > that every review is worth the same. What we lack is not just review > time but qualified review time, ie, comments from someone who's already > familiar with the portion of the code base that's being patched. Right, but I think we're more likely to find such people among the pool of existing contributors than we are among people who don't write patches themselves but happen to volunteer to review. I think Simon's idea of requiring 1 review per patch probably IS a bit overly simplistic - for one thing, someone who submits 10 patches, as I did in the July CommitFest, can scarcely be expected to also review 10 patches. (Even if they were willing, it would make the CommitFest longer, not shorter.) But I don't think they should get by reviewing none, either, especially if they're submitting patches to every CommitFest. It's not my idea that we should punish someone like Dave Page who does a lot of PostgreSQL work and occasionally writes a patch. What I'm complaining about is people who submit patches regularly and rarely or never review. We have enough volunteers to cover new and occasional patch submitters; sometimes those reviews are not quite as thorough, but new and occasional contributors tend to submit relatively simple patches anyway, so it's not a catastrophe. It's the regular patch submitters who, IMHO, most need to be involved. ...Robert
Robert Haas wrote: > On Fri, Nov 13, 2009 at 10:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Simon Riggs <simon@2ndQuadrant.com> writes: >>> All the CF manager needs to do is ensure that every patch submitted >>> chalks up one review. If you think about it, we wouldn't actually need >>> any rr reviewers at all then, because if we have 20 patches we would >>> have 20 reviews due. So the whole scheme is self-balancing. >> Well, no, that's *far* too optimistic/simplistic, because it imagines >> that every review is worth the same. What we lack is not just review >> time but qualified review time, ie, comments from someone who's already >> familiar with the portion of the code base that's being patched. > > Right, but I think we're more likely to find such people among the > pool of existing contributors than we are among people who don't write > patches themselves but happen to volunteer to review. > > I think Simon's idea of requiring 1 review per patch probably IS a bit > overly simplistic - for one thing, someone who submits 10 patches, as > I did in the July CommitFest, can scarcely be expected to also review > 10 patches. (Even if they were willing, it would make the CommitFest > longer, not shorter.) But I don't think they should get by reviewing > none, either, especially if they're submitting patches to every > CommitFest. > > It's not my idea that we should punish someone like Dave Page who does > a lot of PostgreSQL work and occasionally writes a patch. What I'm > complaining about is people who submit patches regularly and rarely or > never review. We have enough volunteers to cover new and occasional > patch submitters; sometimes those reviews are not quite as thorough, > but new and occasional contributors tend to submit relatively simple > patches anyway, so it's not a catastrophe. It's the regular patch > submitters who, IMHO, most need to be involved. I think we (the commitfest manager?) should simply send polite message to any regulars who submits patches but hasn't volunteered for review. Along the lines of: "Hi XXX. You've submitted a patch to the commitfest. As you know, this is a community process and the it depends on volunteers like you. It would be extremely helpful if you could pick a patch that interests you from the list at commitfest.postgresql.org, mark yourself as a reviewer, and review and/or test it as thoroughly as you can. Remember that the faster other patches are reviewed, the faster others get to review your patch and the faster the commitfest can be closed and the version can be released." The commitfest manager can apply common sense: if there's plenty of reviewers already and not many patches, there's no need to reach out to more people. OTOH, if there's a shortage, he can e.g. go through old commitfests too and beg people who have contributed in the past. Think of a fundraiser who calls around people, begging for donations. This is the same, except that we're begging for people's time instead of money. Or think of donating blood. If there's a shortage of blood of a certain type, they will send emails to past donors, asking to come donate. I agree with Tom though that we don't really need a huge pool of people who chip in with one hour per month. We need people who know the codebase pretty well, and who can spend a fair amount of time to do thorough review of complex patches. There is a few dozen or so people out there who have enough knowledge on various parts of the system, or are good at writing documentation, or good at testing. The question is how to get them to donate more time. I'm proposing that we ask them to. It can't hurt. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Simon Riggs wrote: > On Fri, 2009-11-13 at 09:31 -0500, Bruce Momjian wrote: > > > Well, right now we ask for docs, but if they are not supplied, I think > > we just write them ourselves. Is a different enforcement method being > > suggested here? > > And we never bump late patches, nor reject them if sent in missing > format etc. > > We enforce all manner of rules. You tell me this is your main function > on the project even. Is there now no enforcement of any rule, just > because I propose a new one? Enforcing civil behavior in the group, in a private way, is different from having a public policy that requires patch review. Again, I am not against this change --- I am just pointing out it is new territory for us. > Address the main question: how will we get more review time? There are > many other possible proposals, so please lets hear them. (And how would > they be enforced?) Not sure, but I would like to point out the bottleneck is not currently the reviewers but the ability to give final approval and commit it. And again, the commit process is very fast --- it is that final approval that is hard. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
2009/11/14 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>: > I think we (the commitfest manager?) should simply send polite message > to any regulars who submits patches but hasn't volunteered for review. > Along the lines of: > I certainly endorse Heikki's suggestion, but I wonder if we can do more to make reviewing patches an attractive option. As Tom notes, reviewing somebody else's patch just isn't as much fun as working on your own. Robert notes that reviewing patches is a great way to learn about the codebase, and I concur. But in terms of hacker satisfaction it just doesn't compare to creating something, and I think the ratios of submitters to reviewers in recent CFs vindicate me here. I'm thinking of something like a Reviewer Hall of Fame, or Honour Roll. During and after a commitfest, it shows how many reviews have been completed by each person [1]. This could be included in the Weekly News at the CF's conclusion. One of the things that people get out of contributing to an OSS project is the recognition of their peers. Well then, let's leverage that by acclaiming the people who put in a lot of effort reviewing, loudly and publicly. The louder and more public, the more powerful the incentive. Cheers, BJ [1] perhaps with some subjective weighting with respect to patch complexity / depth of review.
Robert Haas wrote: > Please don't sabotage my effort to ensure > an adequate supply of reviewers unless you have a competing proposal. > > I don't think you can reasonably demand this. If I don't think your suggestion is going to improve matters I have a right to say so. cheers andrew
On Fri, Nov 13, 2009 at 12:18 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > I agree with Tom though that we don't really need a huge pool of people > who chip in with one hour per month. We need people who know the > codebase pretty well, and who can spend a fair amount of time to do > thorough review of complex patches. There is a few dozen or so people > out there who have enough knowledge on various parts of the system, or > are good at writing documentation, or good at testing. The question is > how to get them to donate more time. I'm proposing that we ask them to. > It can't hurt. I agree. ...Robert
On Fri, Nov 13, 2009 at 12:32 PM, Brendan Jurd <direvus@gmail.com> wrote: > I'm thinking of something like a Reviewer Hall of Fame, or Honour > Roll. During and after a commitfest, it shows how many reviews have > been completed by each person [1]. > > This could be included in the Weekly News at the CF's conclusion. > > One of the things that people get out of contributing to an OSS > project is the recognition of their peers. Well then, let's leverage > that by acclaiming the people who put in a lot of effort reviewing, > loudly and publicly. The louder and more public, the more powerful > the incentive. I think this would be a nice thing to do. +1 for considering complexity of patches in making this determination. ...Robert
Simon Riggs wrote: > All the CF manager needs to do is ensure that every patch submitted > chalks up one review. If you think about it, we wouldn't actually need > any rr reviewers at all then, because if we have 20 patches we would > have 20 reviews due. So the whole scheme is self-balancing In fact, just suggesting the guideline that everyone who submits a patch should review one here was sufficient to pull in a number of submitters who volunteered to do a single review as well, moving some distance toward what you're describing. It seems we had a perception here that joining rrreviewers "subscribed" you to doing multiple patch reviews; I've let multiple submitters who were trying to help out know it's OK to just grab one patch and review without even getting involved on that list. Take a look at https://commitfest.postgresql.org/action/commitfest_view?id=4 right now. I've been suggesting to people that they assign themselves to the patches they like, and it's nearing completely populated two days before the CommitFest has even started. I have 6 reviewers that haven't been assigned anything yet and there are only 8 unassigned patches out there. In several cases, assigning the reviewer turned out to be quite easy because so many submitters joined in--just assign someone who submitted a patch in the same area. So it far it looks sufficient to introduce the expectation that submitters should also do a review, without even needing to make that a firm rule. That helps increase the reviewer pool significantly, addressing the general problem Robert has been fighting, while not forcing people like Dave who have other pulls on their time into a review role. We'll see whether the follow-through here is good or not, maybe this will decay yet. For now, simply telling submitters that the review of their own patches might be influenced by whether they do a good job reviewing someone else's has improved things considerably over past CommitFests, and it's hard to imagine how someone could complain about a guideline that fair. The most difficult part here remains finding reviewers for the really big patches. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Fri, Nov 13, 2009 at 12:37 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > Robert Haas wrote: >> Please don't sabotage my effort to ensure >> an adequate supply of reviewers unless you have a competing proposal. > > I don't think you can reasonably demand this. If I don't think your > suggestion is going to improve matters I have a right to say so. I've never disputed the right of you or anyone else to say whatever they like. Just to be clear, I don't think that mandating reviews is the best idea anyone has ever had, and I don't rule out the possibility that in solving one problem it might create some others. I think those problems are likely solvable, but I might be wrong, and in any event, it's clearly better for it to be a voluntary system. As far as I can tell, the major objection to having it be mandatory is that it might drive some people away. My major argument for why that isn't the case is that the mere fact that we are even *discussing* whether it should be mandatory has led to a bumper crop of reviewers, including several of the people who fall into the category I've been discussing. So maybe we don't need to make it mandatory: maybe we just need to discuss making it mandatory every 6 months or so. :-) Anyhow, as Bruce pointed out on another message, in some sense we are getting sidetracked. Good reviewers opting out of the system *is* a problem, but lack of a sufficient number of sufficiently involved committers is a bigger one. ...Robert
On Fri, November 13, 2009 1:04 pm, Robert Haas wrote: > the mere fact that we are even *discussing* > whether it should be mandatory has led to a bumper crop of reviewers, Non sequitur. I think it is more likely that the "bumper crop of reviewers" is due to the lengthy discussion about the lack of reviewers. -- nathan wagner
On Fri, 2009-11-13 at 10:12 -0500, Robert Haas wrote: > Keep in mind that > this is a problem that *does not apply to you*. You are a committer. > If no one reviews your patch, you will eventually go ahead and commit > it anyway. If no one reviews my patch, it doesn't go in. That is the problem. I understand that committers may not care, or agree. But blindness to a problem doesn't mean it doesn't exist. Making anyone a committer doesn't change that problem, it just changes *who* experiences the problem. But my view is that if *any* developer experiences a problem then *we* the community experience a problem. -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-13 at 10:55 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > All the CF manager needs to do is ensure that every patch submitted > > chalks up one review. If you think about it, we wouldn't actually need > > any rr reviewers at all then, because if we have 20 patches we would > > have 20 reviews due. So the whole scheme is self-balancing. > > Well, no, that's *far* too optimistic/simplistic, because it imagines > that every review is worth the same. What we lack is not just review > time but qualified review time, ie, comments from someone who's already > familiar with the portion of the code base that's being patched. I agree your point, but the principle is clear. Give back what you have received. If somebody has submitted a complex patch then we expect them to take a complex review. Once the principle has been established, we will all follow it...and my point is...sponsors will then also be forced to follow this also. Dave may worry I am discussing his company. Actually, I'm not. I'm more worried about my sponsors. If I am forced to do review, then I will have to do it. If it is optional, then my sponsors will not pay for it. End of story. Then we end up with a patch that is only reviewed if others agree to do so. Sponsors don't win, but they can't see why. I don't believe that you will personally fill the gaps in our review process for ever and ever: that *would* be optimism. We need a system that works in the longer term. > Now when the current reviewing process was proposed, there were two > separate goals in mind. One was to take whatever incremental load > we could off the eventual committer's work, by catching obvious > mistakes, making sure the docs were up to snuff, etc. The other was > the idea that reviewing would in itself improve the skills of our > development community, by making people read code that they wouldn't > have read otherwise, and that eventually we'd have more committer-grade > people just because of all the reviewing they'd done. (The jury is > still out on whether that will work, but in any case it's a long-term > project.) > > The problem at the moment seems to not be lack of first-level review > time but lack of qualified review. I don't know what we do about that. > Requiring people who have submitted one or two patches to do reviews > isn't going to produce more of the latter, it's going to produce more > of the former. Especially if the patches available to be reviewed > are working in areas they haven't looked at before. Review more, and we get better at it. Practice is the only way I know to get better at something. -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-13 at 10:46 -0500, Robert Haas wrote: > To put this another way, if everyone who submitted a patch reviewed a > patch, we could finish up each CommitFest in 2-3 weeks instead of a > whole month Agreed. That's the idea, lets go with it to see if it works. -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-13 at 12:56 -0500, Greg Smith wrote: > For now, simply telling submitters that the > review of their own patches might be influenced by whether they do a > good job reviewing someone else's has improved things considerably > over past CommitFests, and it's hard to imagine how someone could > complain about a guideline that fair. OK, I guess if we're already doing it... -- Simon Riggs www.2ndQuadrant.com
On Friday 13 November 2009 18:56:01 Greg Smith wrote: > Take a look at > https://commitfest.postgresql.org/action/commitfest_view?id=4 right > now. I've been suggesting to people that they assign themselves to the > patches they like, and it's nearing completely populated two days before > the CommitFest has even started. I have 6 reviewers that haven't been > assigned anything yet and there are only 8 unassigned patches out > there. In several cases, assigning the reviewer turned out to be quite > easy because so many submitters joined in--just assign someone who > submitted a patch in the same area. I have to admit that at least for me personally its way much easer to get started on a patch one finds interesting than when not - especially if the reviewing time slot is after a 10-12h workday or such. To the point where starting to review a personally "boring" patch takes more time than the review would have taken itself. Embarassed, Andres
On Fri, 2009-11-13 at 23:10 +0100, Andres Freund wrote: > I have to admit that at least for me personally its way much easer to get > started on a patch one finds interesting than when not I find it much easier to get interested in a patch after I get started reviewing it ;) Seriously, that's happened with at least a couple patches that have been assigned to me. Either the code itself is interesting, or the feature is useful enough that I would want to learn about it anyway. Regards,Jeff Davis
Robert Haas wrote: > Anyhow, as Bruce pointed out on another message, in some sense we are > getting sidetracked. Good reviewers opting out of the system *is* a > problem, but lack of a sufficient number of sufficiently involved > committers is a bigger one. I want to thank everyone for the fine discussion we are having about the community patch review and commit process. I am somewhat relieved that others share the concerns I expressed earlier. Having read the discussion and heard people's opinions, I am now thinking that I need to get more involved in committing patches. I used to do a lot of that, but backed away from it when the commit fest process started so the new system could develop on its own, without being swayed by my involvement. It also coincided with the start of a heavy travel period for me and involvement in side projects, like pg_migrator. I am probably more able than most to adjust my schedule to devote time to committing things. I am not great at committing patches, but with the help of others, I think I can help carry the load. Ironically, I am leaving tomorrow for a trip to Japan and China, and return on November 26 (Thanksgiving in the USA), so the bad news is I can't help much until I return. The good news is that I don't have any trips scheduled after that, and trips are usually scheduled months in advance, so it is unlikely I will be traveling much in the coming months. I am thinking I need to travel less and focus on helping commit patches. We now have a very capable world-wide group of Postgres community speakers so my decline from the conference circuit should have little impact. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Fri, Nov 13, 2009 at 11:06 PM, Bruce Momjian <bruce@momjian.us> wrote: > Having read the discussion and heard people's opinions, I am now > thinking that I need to get more involved in committing patches. Woot. ...Robert
Bruce Momjian wrote: > I am probably more able than most to adjust my schedule to devote time > to committing things. > Yes, time is what has restricted what I can do. I'll try to do a bit more for this coming commitfest, but I'm rather sad that I haven't made a more substantial contribution to this release. cheers andrew
andrew@dunslane.net (Andrew Dunstan) writes: > Robert Haas wrote: >> I am personally quite tired of reviewing patches for people who don't >> in turn review mine (or someone's). It makes me feel like not >> working on this project. If we can solve that problem without >> implementing a policy of this type, that is good. I would much >> prefer to run by the honor system rather than having to threaten to >> drop patches, but only if the honor system actually works. > > Organizing contributors on a project like this is like herding > cats. Threats and penalties are unlikely to be effective. This is > essentially a charity where people give in ways that work for them, > and you take whatever they have to give. I'm extremely uncomfortable > with the idea of a prescriptive system. I've proposed them myself in > the past, but I have since come to the realization that it will simply > drive people away. Ah, but the thing is, what was proposed wasn't "totally evilly draconian." There's a difference between: "You haven't reviewed any patches - we'll ignore you forever!" and "Since you haven't reviewed any patches, we are compelled to defer your patches until the next CommitFest." It's enough pain to make people think, but it's not *totally* punitive. -- "I really only meant to point out how nice InterOp was for someone who doesn't have the weight of the Pentagon behind him. I really don't imagine that the Air Force will ever be able to operate like a small, competitive enterprise like GM or IBM." -- Kent England
On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote: > Ah, but the thing is, what was proposed wasn't "totally evilly > draconian." > > There's a difference between: > > "You haven't reviewed any patches - we'll ignore you forever!" > > and > > "Since you haven't reviewed any patches, we are compelled to defer your > patches until the next CommitFest." > > It's enough pain to make people think, but it's not *totally* punitive. It is important to remember we are all volunteers here. Any increase to the barrier of contribution is a bad one. Joshua D. Drake -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Mon, Nov 16, 2009 at 12:17 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote: > >> Ah, but the thing is, what was proposed wasn't "totally evilly >> draconian." >> >> There's a difference between: >> >> "You haven't reviewed any patches - we'll ignore you forever!" >> >> and >> >> "Since you haven't reviewed any patches, we are compelled to defer your >> patches until the next CommitFest." >> >> It's enough pain to make people think, but it's not *totally* punitive. > > It is important to remember we are all volunteers here. Any increase to > the barrier of contribution is a bad one. True. But "not enough reviewers to review all the patches we get" is also a barrier to contribution. ...Robert
jd@commandprompt.com ("Joshua D. Drake") writes:
> On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
>
>> Ah, but the thing is, what was proposed wasn't "totally evilly
>> draconian."
>>
>> There's a difference between:
>>
>> "You haven't reviewed any patches - we'll ignore you forever!"
>>
>> and
>>
>> "Since you haven't reviewed any patches, we are compelled to defer
>> your patches until the next CommitFest."
>>
>> It's enough pain to make people think, but it's not *totally*
>> punitive.
>
> It is important to remember we are all volunteers here. Any increase to
> the barrier of contribution is a bad one.
But this *isn't* a barrier to contribution, at least not notably more
than the already existant issue that a paucity of reviewers is a barrier
to contribution.
It represents a policy for triaging review efforts with a bias in favor
of those that *are* contributing to the reviewers' list.
I don't think it's unjust for those that contribute to the review
process to get more favorable scheduling of reviews to their patches.
If we get so many reviewers that such triaging becomes unnecessary, then
it may automatically *not* be a problem.
--
(format nil "~S@~S" "cbbrowne" "acm.org")
http://linuxfinances.info/info/slony.html
"Bother," said Pooh, as he deleted his root directory.
On Mon, 2009-11-16 at 12:42 -0500, Robert Haas wrote: > On Mon, Nov 16, 2009 at 12:17 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote: > > > >> Ah, but the thing is, what was proposed wasn't "totally evilly > >> draconian." > >> > >> There's a difference between: > >> > >> "You haven't reviewed any patches - we'll ignore you forever!" > >> > >> and > >> > >> "Since you haven't reviewed any patches, we are compelled to defer your > >> patches until the next CommitFest." > >> > >> It's enough pain to make people think, but it's not *totally* punitive. > > > > It is important to remember we are all volunteers here. Any increase to > > the barrier of contribution is a bad one. > > True. But "not enough reviewers to review all the patches we get" is > also a barrier to contribution. No. It is a barrier of contribution not to contribution. The types of current structure that are being considered are punitive regardless of the softness of wording. This is certainly not an easy problem to solve and I am not saying I have a better solution (although something more personal and direct such as the way Selena helps user groups seems more appropriate). Sincerely, Joshua D. Drake > > ...Robert > -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Mon, Nov 16, 2009 at 1:08 PM, Joshua D. Drake <jd@commandprompt.com> wrote: >> True. But "not enough reviewers to review all the patches we get" is >> also a barrier to contribution. > > No. It is a barrier of contribution not to contribution. I am not sure exactly what that means, but I agree that it isn't quite the same. Backing up a minute, AIUI, the CommitFest process was created to solve the problem that patches weren't getting reviewed in a timely fashion. To address that problem, dedicated times were created in each release cycle for people to stop working on their own patches and review patches from other contributors. I haven't been around long enough to be able to compare from personal experience, but I think generally what I've heard is that the new process is a big improvement. But, there are some problems, and speaking from experience, one of those problems is that reviewing patches and running CommitFests is long, hard, and difficult when not enough people volunteer to review, or not enough committers volunteer to commit. I guess I agree with your statement that the structures that are being proposed are punitive, although perhaps I might choose the word coercive instead. Clearly, the preferable solution is for people to volunteer. But if they don't, we haven't got a lot of options. Perhaps by encouraging them to volunteer and recognizing their contributions when they do volunteer, we can get the number of volunteers back up to an adequate level. If after doing those things we still don't have enough volunteers, we're not going to be able to review all the patches. Should that occur, we'll have to decide which ones to review and which ones to skip. Maybe we'll just let people volunteer and any patches for which nobody volunteers will fall on the floor or be forever postponed to the next CommitFest. Maybe we'll try to assign reviewers preferentially to first-time contributors and those who are themselves reviewing, as I'm suggesting. Or maybe we'll handle it some other way. I don't know. It seems we don't have to decide yet. ...Robert
On Mon, Nov 16, 2009 at 12:41:02PM -0500, Chris Browne wrote:
> jd@commandprompt.com ("Joshua D. Drake") writes:
> > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote:
> >
> >> Ah, but the thing is, what was proposed wasn't "totally evilly
> >> draconian."
> >>
> >> There's a difference between:
> >>
> >> "You haven't reviewed any patches - we'll ignore you forever!"
> >>
> >> and
> >>
> >> "Since you haven't reviewed any patches, we are compelled to
> >> defer your patches until the next CommitFest."
> >>
> >> It's enough pain to make people think, but it's not *totally*
> >> punitive.
> >
> > It is important to remember we are all volunteers here. Any
> > increase to the barrier of contribution is a bad one.
>
> But this *isn't* a barrier to contribution, at least not notably
> more than the already existant issue that a paucity of reviewers is
> a barrier to contribution.
>
> It represents a policy for triaging review efforts with a bias in
> favor of those that *are* contributing to the reviewers' list.
>
> I don't think it's unjust for those that contribute to the review
> process to get more favorable scheduling of reviews to their
> patches.
>
> If we get so many reviewers that such triaging becomes unnecessary,
> then it may automatically *not* be a problem.
In the PostgreSQL Weekly News, I track patches, and apparently at
least one person reads that section. Would it be helpful to track
reviews somehow during commitfests with the reviewers' names
prominently attached?
It's a more positive approach, and like many others, I really prefer
those types of approaches, even if I grump occasionally. :)
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
2009/11/17 David Fetter <david@fetter.org>: > In the PostgreSQL Weekly News, I track patches, and apparently at > least one person reads that section. Would it be helpful to track > reviews somehow during commitfests with the reviewers' names > prominently attached? > Yes. See also my suggestion [1] that we do a Reviewer "Honour Roll" or "Hall of Fame" at the end of the CF, also published in the PWN. One of the rewards for getting a patch into the tree is having your name immortalised in the commit log. There's no such compensation for reviewing patches. I think creating incentives to review is going to be more potent and more enjoyable for everyone involved than punitive measures. Cheers, BJ [1] http://archives.postgresql.org/message-id/37ed240d0911130932i3b48849csb8cbae061abf11e4@mail.gmail.com
Brendan Jurd <direvus@gmail.com> writes:
> One of the rewards for getting a patch into the tree is having your
> name immortalised in the commit log. There's no such compensation for
> reviewing patches.
Well, that could be fixed: instead of
blah blah blah
Joe Coder
we could write
blah blah blah
Joe Coder, reviewed by X and Y
Although keeping track of just who to credit might be a bit tricky.
I'd be happy to commit to crediting whoever is listed in the CF entry
for the patch, but sometimes other people have chimed in as much or
more as the nominal reviewer.
regards, tom lane
Brendan Jurd wrote: > One of the rewards for getting a patch into the tree is having your > name immortalised in the commit log. There's no such compensation for > reviewing patches. > > I think creating incentives to review is going to be more potent and > more enjoyable for everyone involved than punitive measures. > > > Indeed. I once suggested only half jokingly that we should have a "Coder of the month" award. Maybe a "Reviewer of the month" award would also be good. Seriously, the major benefit most people get from contributing (apart from good karma and a warm inner glow) is kudos, and we should possibly lay it on a bit thicker. cheers andrew
On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote: > Ah, but the thing is, what was proposed wasn't "totally evilly > draconian." > > There's a difference between: > > "You haven't reviewed any patches - we'll ignore you forever!" > > and > > "Since you haven't reviewed any patches, we are compelled to defer your > patches until the next CommitFest." > > It's enough pain to make people think, but it's not *totally* punitive. It is important to remember we are all volunteers here. Any increase to the barrier of contribution is a bad one. Joshua D. Drake -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Mon, 2009-11-16 at 12:42 -0500, Robert Haas wrote: > On Mon, Nov 16, 2009 at 12:17 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > > On Mon, 2009-11-16 at 11:31 -0500, Chris Browne wrote: > > > >> Ah, but the thing is, what was proposed wasn't "totally evilly > >> draconian." > >> > >> There's a difference between: > >> > >> "You haven't reviewed any patches - we'll ignore you forever!" > >> > >> and > >> > >> "Since you haven't reviewed any patches, we are compelled to defer your > >> patches until the next CommitFest." > >> > >> It's enough pain to make people think, but it's not *totally* punitive. > > > > It is important to remember we are all volunteers here. Any increase to > > the barrier of contribution is a bad one. > > True. But "not enough reviewers to review all the patches we get" is > also a barrier to contribution. No. It is a barrier of contribution not to contribution. The types of current structure that are being considered are punitive regardless of the softness of wording. This is certainly not an easy problem to solve and I am not saying I have a better solution (although something more personal and direct such as the way Selena helps user groups seems more appropriate). Sincerely, Joshua D. Drake > > ...Robert > -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Mon, 2009-11-16 at 19:15 -0500, Andrew Dunstan wrote: > > Brendan Jurd wrote: > > One of the rewards for getting a patch into the tree is having your > > name immortalised in the commit log. There's no such compensation for > > reviewing patches. > > > > I think creating incentives to review is going to be more potent and > > more enjoyable for everyone involved than punitive measures. > > > > > > > > Indeed. I once suggested only half jokingly that we should have a "Coder > of the month" award. Maybe a "Reviewer of the month" award would also be > good. Seriously, the major benefit most people get from contributing > (apart from good karma and a warm inner glow) is kudos, and we should > possibly lay it on a bit thicker. In the old days (I can't believe I said that), it was not uncommon for a developer to just want a thank you, some pizza and beer. I don't know that it is much different now. It is amazing what people are willing to do if they feel a little appreciated. Joshua D. Drake > > cheers > > andrew > -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Nov 16, 2009, at 8:47 PM, "Joshua D. Drake" <jd@commandprompt.com> wrote: > On Mon, 2009-11-16 at 19:15 -0500, Andrew Dunstan wrote: >> >> Brendan Jurd wrote: >>> One of the rewards for getting a patch into the tree is having your >>> name immortalised in the commit log. There's no such compensation >>> for >>> reviewing patches. >>> >>> I think creating incentives to review is going to be more potent and >>> more enjoyable for everyone involved than punitive measures. >>> >>> >>> >> >> Indeed. I once suggested only half jokingly that we should have a >> "Coder >> of the month" award. Maybe a "Reviewer of the month" award would >> also be >> good. Seriously, the major benefit most people get from contributing >> (apart from good karma and a warm inner glow) is kudos, and we should >> possibly lay it on a bit thicker. > > In the old days (I can't believe I said that), it was not uncommon > for a > developer to just want a thank you, some pizza and beer. I don't know > that it is much different now. > > It is amazing what people are willing to do if they feel a little > appreciated. +1. ...Robert > >>
On Mon, Nov 16, 2009 at 6:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Brendan Jurd <direvus@gmail.com> writes: >> One of the rewards for getting a patch into the tree is having your >> name immortalised in the commit log. There's no such compensation for >> reviewing patches. > > Well, that could be fixed: instead of > > blah blah blah > > Joe Coder > > we could write > > blah blah blah > > Joe Coder, reviewed by X and Y > > Although keeping track of just who to credit might be a bit tricky. > I'd be happy to commit to crediting whoever is listed in the CF entry > for the patch, but sometimes other people have chimed in as much or > more as the nominal reviewer. If looking at the CF entries, it's important to note that sometimes one person posts a review and someone else (historically, me) adds a link to it, and of course we want to post the reviewer, not the person who dropped in the link. I try to always make the comment something like "review from so-and-so" when I do this, but I (or someone) might forget that on occasion, so clicking through to the underlying message is probably a good idea. As for other people chiming in, I think half a loaf is better than none. We should try to credit the people who deserve credit; and if someone who chimes in is particularly concerned about getting credit, then they can post a link to their chiming-in on the CF app. Otherwise, it's best effort, just like anything else. ...Robert
On Mon, 2009-11-16 at 19:15 -0500, Andrew Dunstan wrote: > > Brendan Jurd wrote: > > One of the rewards for getting a patch into the tree is having your > > name immortalised in the commit log. There's no such compensation for > > reviewing patches. > > > > I think creating incentives to review is going to be more potent and > > more enjoyable for everyone involved than punitive measures. > > > > > > > > Indeed. I once suggested only half jokingly that we should have a "Coder > of the month" award. Maybe a "Reviewer of the month" award would also be > good. Seriously, the major benefit most people get from contributing > (apart from good karma and a warm inner glow) is kudos, and we should > possibly lay it on a bit thicker. In the old days (I can't believe I said that), it was not uncommon for a developer to just want a thank you, some pizza and beer. I don't know that it is much different now. It is amazing what people are willing to do if they feel a little appreciated. Joshua D. Drake > > cheers > > andrew > -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564 Consulting, Training, Support, Custom Development, Engineering If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
On Nov 17, 2009, at 9:15 AM, Andrew Dunstan wrote: > Indeed. I once suggested only half jokingly that we should have a "Coder of the month" award. I suggest that it be named "The Tom Lane" award, and disqualify Tom from winning (sorry Tom). ;-) David