On 07/05/2013 02:34 PM, Jeff Janes wrote: > On Wed, Jul 3, 2013 at 12:03 PM, Christopher Browne <cbbro...@gmail.com>wrote: > >> On Wed, Jul 3, 2013 at 2:24 PM, Cédric Villemain >> <ced...@2ndquadrant.com>wrote: >>> Others rules appeared, like the 5 days limit. >>> >> > The limit was previously 4 days (at least according to > http://wiki.postgresql.org/wiki/RRReviewers) but I think that that was > honored almost exclusively in the breach. I don't have a sense for how the > 5 day limit is working. If it is working, great. If not, I would advocate > lengthening it--having a limit specified but not generally held to is > counterproductive. I know that I, and at least one other potential > reviewer, won't ask to be assigned a random patch because we have no > confidence we can do an adequate review of a random patch within a > contiguous 5 day window.
Well, keep in mind that the "reviewer" blank on the commitfest page is solely a reservation to prevent duplicate effort. There is absolutely nothing preventing someone from doing and submitting a review without putting their name down -- and several have, this CF. The idea of the 5-day limit is that this often happens: 1. reviewer takes patch because they want to review it 2. something comes up in their life outside the postgres project, and they don't get time to review it 3. because their name is on the patch as "reviewer", others don't pick it up for review 4. it gets to 3 days before CF end, and nobody has reviewed the patch While there are certainly cases where a reviewer takes more than 5 days actively to review a patch, these are the minority. Most of the time, a reviewer who doesn't review a patch within 5 days doesn't get around to reviewing it that commitfest, period. If you are actively reviewing for more than 5 days, simply send me/Mike an email saying so, before the 5 days are up. Or post something on -hackers in the patch thread. Previously, we dealt with this by sending out an "are you reviewing this patch" email from the CFM to the reviewer after 5 days passed. The problem with that is that a reviewer who was too busy to review was usually too busy to answer, and as a result the whole cycle of "he's not gonna review that" took around 10 days on average before we knew we could remove the name. 10 days is 1/3 of the total commitfest -- unacceptably long. Thus the approach we're trying this CF. Again, this is an area where better automation could really help. Reviewers could get automated reminders at 4 and 5 days, and ack those reminders to extend the review period. > On the other hand, I could always just go to the open commitfest (rather > than the in progress one), pick something at random myself, and have up to > 3 months to review it. I just don't do have the discipline to do that, at > least not often. Please do! >> To me it outlines that some are abusing the CF app and pushing there >>> useless >>> patches (not still ready or complete, WIP, ... >> >> >> Seems to me that "useless" overstates things, but it does seem fair to >> say that some patches are not sufficiently well prepared to be efficiently >> added into Postgres. Unfortunately, the only time we guarantee that a patch or even a spec proposal will get a hearing and discussion is the CF. Thus people who really want to get agreement on a prototype, spec, proposal or API are gonna submit it to the CF, so that they get some useful feedback. Most of the time, someone posting a WIP, API, spec, SQL syntax or feature concept outside of a CF gets little or no useful criticism/suggestions on it. If we don't want WIP/RFC patches in the CF, then we need to provide some other way to guarantee that these incomplete patches will get feedback. I'd be in favor of having something -- I think more authors would get better feedback early on in development -- but I have no idea what it would be. Other uncommittable patches submitted to the CF are there because the submitter is sending in a first-time patch. It's very important for training up the new submitter that their patch get the full review-returned-with-feedback cycle. That's how they become better patch authors in the future. Personally, I think this is one of the most valuable aspects of the CF process. >> Well, if the project is hampered by not being able to get *all* the >> changes that people imagine that they want to put in, then we have a >> real problem of needing a sort of "triage" to determine which changes >> will be accepted, and which will not. >> >> Perhaps we need an extra status in the CommitFest application, namely >> one that characterizes: >> Insufficiently Important To Warrant Review Gods forbid. We might as well have the tag "Stupid Idiot Patch" and be done with it. And people accuse *me* of being submitter-hostile. > I don't think that this would really be an improvement. Someone still has > to spend enough time looking at the patch to make the decision that it > falls into one of those categories. Having spent sufficient time to do > that, what they did is ipso facto a review, and they should just set it as > either rejected (not important or idea unworkable) or RwF (idea is > workable, but the given implementation is not). Exactly. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
