Thread: Re: new CommitFest states (was: YAML)
On Mon, Dec 7, 2009 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> It was written and submitted by one person who did not bother to ask >>> first whether anyone else thought it was worthwhile. So its presence >>> on the CF list should not be taken as evidence that there's consensus >>> for it. > >> Should we have "Needs Discussion" phase before "Needs Review" ? >> Reviews, including me, think patches with needs-review status are >> worthwhile. In contrast, contributers often register their patches >> to CF without discussions just because of no response; they cannot >> find whether no response is silent approval or not. > > Hm, I guess the question would be: what is the condition for getting > out of that state? It's clear who is supposed to move a patch out of > 'Needs Review', 'Waiting for Author', or 'Ready for Committer' > respectively. I don't know who's got the authority to decide that > something has or has not achieved community consensus. > > Right at the moment we handle this sort of problem in a very informal > way, but if it's going to become part of the commitfest state for a > patch I think we need to be a bit less informal. This sounds suspiciously like it could lead to a situation where people want to use the CommitFest application to track feature ideas rather than actual patches. The next step would be to require that the "Needs Discussion" status doesn't actually require any code, and the code can be submitted when it reaches "Needs Review". I think this whole line of thinking is a bad one. The CommitFest application isn't designed to manage feature requests, and I don't want to turn it into something that does. We might need a system to manage that, at some point, but let's not shoehorn it into what we have now. The right answer here is that no one should assume that a patch is wanted just because it is in the CommitFest application. This point is actually explicitly addressed in this document http://wiki.postgresql.org/wiki/Reviewing_a_Patch If people submit patches without getting consensus that the feature is something that we want, then the penalty is that their patch is more likely to be rejected. If you're worried about this, then you should get consensus before you spend time writing code. If you're not worried about it, that's your prerogative: experienced patch authors often know what will and will not fly without a drawn-out discussion, especially for very minor enhancements or bug fixes. On a related note, Greg Smith requested a state called "Discussing Review", which would logically follow "Needs Review" and precede "Waiting for Author"/"Ready for Committer"/"Returned with Feedback". I'm not altogether convinced of the value of that state, but I'm not altogether opposed to it either. If we're going to have a discussion of CommitFest states, we probably ought to talk about that one, too. ...Robert
Robert Haas wrote: > On a related note, Greg Smith requested a state called "Discussing > Review", which would logically follow "Needs Review" and precede > "Waiting for Author"/"Ready for Committer"/"Returned with Feedback". > I'm not altogether convinced of the value of that state, but I'm not > altogether opposed to it either. If we're going to have a discussion > of CommitFest states, we probably ought to talk about that one, too. > Don't know what it is about YAML that it encourages slipping into CF management..."Discussing Review" is a state patches seem to fall into for a time. I'd like to see it added mainly because it simplifies work for a lazier (than Robert) CF manager like myself, which I think is a more appropriate target for this process. Some of the process that works for him I don't think can be replicated by other personalities very well. If a patch is being actively discussed on the list, often the author is at the mercy of said discussion ending before they can make another forward step; this is why "Waiting for Author" isn't appropriate. Having the patch sitting in "Needs Review" instead is unfair to the reviewer, who would like to be able to mark it as "I'm done" and move on. That's also why sitting in that state takes up time for the CF manager. They need to scan all "waiting for [author|review]" patches to get a list of people to nudge, and in this case there is no one to nudge--we're all at the mercy of the list to reach some sort of conclusion. The obvious concern here is "who has the action item them?" In this case, that's the CF manager's job--to help wrap the discussion up once it's died down and figure out what state the patch should go into next. Reviewers would mark a patch "Discussing Review" once they're done and have sent their review to the list when it doesn't fit any of the existing next states well, and they're not sure what happens next. Basically it allows them to formally push making a hard decision about a patch upwards, which is effectively what's happening now. Then the CF manager or committer can mark it "returned with feedback" or flat-out "rejected" if the resulting discussion isn't favorable, rather than making the reviewer responsible for that, once discussion has wrapped up. Or the author/CF manager can eventually mark it "waiting for author" once one of them has decided that's the logical next step. I plan to turn the whole thing into a fairly easy to follow state diagram as documentation on the process, there's just this one common state I don't have a label for now: when things are trapped on the list, and nobody has an obvious next move until that settles down. There is no need or want for a "Needs discussion" before review or code submission. That just encourages abusing the CF app and process for something you can do quite nicely with the mailing list. If you believe in a feature but don't want to get community buy-in on the list first, write a patch to prove it works. If the reviewer torpedos your patch, don't expect you'll get a free round of discussing whether everyone wants that feature or not out of the deal. For this YAML example, had the code in the patch been junk we wouldn't even have gotten to discussion of its utility--the reviewer would have rejected it and that would have been it. And that IMHO is how it should be. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
I didn't really get any feedback on my proposal to add a new "Discussing review" state to help out the reviewers and CF manager. To show how adding it helps track the common flow of patches through the system, I turned the whole CF into a big state machine and marked how the transitions should normally happen at http://wiki.postgresql.org/wiki/Running_a_CommitFest If you carefully read the "Followup" section, which Robert wrote initially, you can see it implies such a state exists via the "Bogged down in discussion" comments. I'd just like to have a way to flag patches that are entering discussion because the reviewer isn't sure what happens next as such directly, to make this whole process more mechanical, fair, and predictable to the bulk of the participants (reviewers and authors). I wasn't able to figure out how to do that without adding this additional state to the system. Robert's main objection to this idea, that at any point there should be one obvious person with the next action to take and authors should take more responsibility for their patch, is a good ideal. But since the CF manager ends up being the backstop for breakdowns in the process, they're always a second possible source for transitions. I'd rather them be a more predictable such source for a number of reasons. Note that a major secondary goal here is to make it easier to bring on-board new CF managers and have them hit the ground running, by having near deterministic suggestions for much of what they need to do. It's also not a coincidence that some of the more boring parts of that job step toward being specified well enough that one might start automating them too. How to construct a simple "nag bot" that mails pgsql-rrreviewers to perform most of the CF actions listed here is pretty obvious working from this table. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith <greg@2ndquadrant.com> wrote: > I didn't really get any feedback on my proposal to add a new > "Discussing review" state It seems like a good idea to me; it better models the reality. > http://wiki.postgresql.org/wiki/Running_a_CommitFest It seems to me that a patch could move from "Discussing review" to "Needs review" -- if the reviewer decided to discuss the approach before continuing the review process and the discussion confirms the approach as viable. -Kevin
Kevin Grittner wrote: <blockquote cite="mid:4B260712020000250002D3EF@gw.wicourts.gov" type="cite"><blockquote type="cite"><prewrap=""><a class="moz-txt-link-freetext" href="http://wiki.postgresql.org/wiki/Running_a_CommitFest">http://wiki.postgresql.org/wiki/Running_a_CommitFest</a> </pre></blockquote><prewrap=""> It seems to me that a patch could move from "Discussing review" to "Needs review" -- if the reviewer decided to discuss the approach before continuing the review process and the discussion confirms the approach as viable. </pre></blockquote> In that case, the patch would be in "Needs review" the whole time. "Discussing review"is intended to be a "I'm done but not sure of the next step for this patch" state the reviewer can use. In the situationyou described, the patch would never have left "Needs review". I just made that more clear by documenting thatit's shorthand for "discussing review results".<br /><br /> I also added a transition path for a similar situation though,where the discussion concludes the reviewer didn't do the right thing in the first place (even though they thoughtthey did) and they return to reviewing after realizing what was missing.<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 Mon, Dec 14, 2009 at 12:01 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Kevin Grittner wrote: > > http://wiki.postgresql.org/wiki/Running_a_CommitFest > > > > It seems to me that a patch could move from "Discussing review" to > "Needs review" -- if the reviewer decided to discuss the approach > before continuing the review process and the discussion confirms the > approach as viable. > > > In that case, the patch would be in "Needs review" the whole time. > "Discussing review" is intended to be a "I'm done but not sure of the next > step for this patch" state the reviewer can use. In the situation you > described, the patch would never have left "Needs review". I just made that > more clear by documenting that it's shorthand for "discussing review > results". > > I also added a transition path for a similar situation though, where the > discussion concludes the reviewer didn't do the right thing in the first > place (even though they thought they did) and they return to reviewing after > realizing what was missing. I don't think there should be a transition from Returned with Feedback back to Waiting for review. Granted we might allow that occasionally as an exceptional case, but normally Returned with Feedback is a final state. (Also, Waiting for review is actually the wrong name for the state it's trying to talk about.) ...Robert
Robert Haas wrote: > I don't think there should be a transition from Returned with Feedback > back to Waiting for review. Granted we might allow that occasionally > as an exceptional case, but normally Returned with Feedback is a final > state. > I did throw some disclaimers in the notes about this particular subject at the bottom of the table. The main reason I put that in there is that sometimes a reviewer or even the CF manager (I did this myself once this time) will mark something "Returned with feedback", thinking there's no way the issues pointed out can be addressed right now. And then, a day or two later, in comes a patch that does just that; surprise! Since it seems to happen anyway, and I'd prefer not to get in the position where people are screaming "you threw me out with 'RWF' unfairly", I thought it was better to accept that possibility so long as the whole thing is tightly bounded as far as how much time the author has to do it. > (Also, Waiting for review is actually the wrong name for the state > it's trying to talk about.) > Uh, what are you talking about here? -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Mon, Dec 14, 2009 at 12:38 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Robert Haas wrote: >> I don't think there should be a transition from Returned with Feedback >> back to Waiting for review. Granted we might allow that occasionally >> as an exceptional case, but normally Returned with Feedback is a final >> state. > > I did throw some disclaimers in the notes about this particular subject at > the bottom of the table. The main reason I put that in there is that > sometimes a reviewer or even the CF manager (I did this myself once this > time) will mark something "Returned with feedback", thinking there's no way > the issues pointed out can be addressed right now. And then, a day or two > later, in comes a patch that does just that; surprise! Since it seems to > happen anyway, and I'd prefer not to get in the position where people are > screaming "you threw me out with 'RWF' unfairly", I thought it was better to > accept that possibility so long as the whole thing is tightly bounded as far > as how much time the author has to do it. Hmm, I'm not aware of any actual cases of this. I'm usually pretty conservative about jumping to RWF unless there's been lag or we're near the end of the CommitFest, so it doesn't come up. >> (Also, Waiting for review is actually the wrong name for the state >> it's trying to talk about.) > > Uh, what are you talking about here? Well, we have Needs Review and Waiting on Author, but not Waiting for Review. I assume you mean Needs Review. ...Robert
Robert Haas wrote: > On Mon, Dec 14, 2009 at 12:38 PM, Greg Smith <greg@2ndquadrant.com> wrote: > >> Robert Haas wrote: >> >>> I don't think there should be a transition from Returned with Feedback >>> back to Waiting for review. Granted we might allow that occasionally >>> as an exceptional case, but normally Returned with Feedback is a final >>> state. >>> >> The main reason I put that in there is that >> sometimes a reviewer or even the CF manager (I did this myself once this >> time) will mark something "Returned with feedback", thinking there's no way >> the issues pointed out can be addressed right now. And then, a day or two >> later, in comes a patch that does just that; surprise! > Hmm, I'm not aware of any actual cases of this. I'm usually pretty > conservative about jumping to RWF unless there's been lag or we're > near the end of the CommitFest, so it doesn't come up. > I've concluded that the times this happened was just me being too aggressive here to close some patches out after getting behind, and I removed the path you objected to out of the page as not to encourage that behavior. I think that http://wiki.postgresql.org/wiki/Running_a_CommitFest makes for a pretty reasonable and quite detailed set of guidelines now for the whole process, which means we've successfully gotten "what Robert did to make things work well" documented fully. All it's missing is for the "Discussing review" state to be an official one. I could undo things back to where it's not listed, but I do think it matches what we really do better and might as well be recognized as such. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com