Thread: Review request: XLogInsert
It looks like our original reviewer for the "XLogInsert: move some work out of critical section" isn't going to be able to handle that, so we're back to needing one more first round review. This is a moderately complicated patch (based on the complexity of the code it modifies rather than its size) that has a performance testing component to it. Are any of the people who suggested they'd be free in December available at this point interested? I'm hoping we're close to having all initial reviews done here this week. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Dec 6, 2009, at 10:24 AM, Greg Smith <greg@2ndquadrant.com> wrote: > Are any of the people who suggested they'd be free in December > available at this point interested? I'm hoping we're close to > having all initial reviews done here this week. Not to distract from the issue at hand, but that goal doesn't seem quite aggressive enough, considering that this is the last week of the CommitFest, or near enough. Do you have a plan for wrapping this up? ...Robert
Robert Haas wrote:
> Not to distract from the issue at hand, but that goal doesn't seem
> quite aggressive enough, considering that this is the last week of the
> CommitFest, or near enough. Do you have a plan for wrapping this up?
To step back for a second, the fact that I have to create a plan shows a
failure in our getting this turned into a real process. That what I've
been trying to do here--step back each week and figure out what I should
have done, and try to make it more likely that will happen the next
time. We should have a clear plan charted that says what will happen at
each step here.
It looks to me like the end of the previous CF work finished via a huge
amount of "patch chasing" work from you and possibly other helpers (I
don't know, as I haven't gotten any such help myself the last few
weeks). That was fine since you were blazing a trail here, but that's
not a sustainable model. This whole thing needs clear written deadlines
and process if it's going to run more automatically in the future. CF
manager and helper labor isn't easy to find an unlimited amount of. I'd
like to turn this all into something more like a state machine whose
transitions are marked out on the calendar from day one.
At around three weeks, where we're at now, I think what should happen
next is:
1) All "waiting for author patches" turn into "returned with feedback"
as of some deadline. Since there wasn't one in advance, maybe I
announce one on the hackers list today?
2) Poll the reviewer of every patch that's had an updated version who
hasn't submitted a re-review asking whether they think that version is
"ready for comitter" now, if they have more feedback, or if they feel
it's just not ready yet and should be rejected. In any case but "ready
for committer", it goes into "returned with feedback" pile.
3) Any patches in this state that we haven't heard back from the
reviewer on within a couple of days get decided on ("ready" /
"returned") at the CommitFest manager's discretion. If anyone feels
wronged by that, they can always ask that a committer take a look
anyway. The CF manager won't always have as much information as we
expect the reviewers to, and can be presumed to have a thicker skin
about people getting mad at them for making a bad decision too.
I have a deliverable to ship today, once I'm done with that I'll start
rattling people more. Feedback about tweaking the above before I start
executing on it would be appreciated.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
On Sun, Dec 6, 2009 at 12:18 PM, Greg Smith <greg@2ndquadrant.com> wrote:
> Robert Haas wrote:
>>
>> Not to distract from the issue at hand, but that goal doesn't seem quite
>> aggressive enough, considering that this is the last week of the CommitFest,
>> or near enough. Do you have a plan for wrapping this up?
>
> To step back for a second, the fact that I have to create a plan shows a
> failure in our getting this turned into a real process. That what I've been
> trying to do here--step back each week and figure out what I should have
> done, and try to make it more likely that will happen the next time. We
> should have a clear plan charted that says what will happen at each step
> here.
I made an effort to document this in the section of the "Running a
CommitFest" page that is entitled "Followup, Followup, Followup". But
that effort may not have been as successful as might have been wished,
and I apologize for that, welcome your efforts to rectify the
situation, and wish to help if I can.
> It looks to me like the end of the previous CF work finished via a huge
> amount of "patch chasing" work from you and possibly other helpers (I don't
> know, as I haven't gotten any such help myself the last few weeks). That
> was fine since you were blazing a trail here, but that's not a sustainable
> model. This whole thing needs clear written deadlines and process if it's
> going to run more automatically in the future. CF manager and helper labor
> isn't easy to find an unlimited amount of. I'd like to turn this all into
> something more like a state machine whose transitions are marked out on the
> calendar from day one.
I'm not sure that's going to be possible, but I certainly think that
reducing the amount of labor involved is a good goal.
> At around three weeks, where we're at now, I think what should happen next
> is:
>
> 1) All "waiting for author patches" turn into "returned with feedback" as of
> some deadline. Since there wasn't one in advance, maybe I announce one on
> the hackers list today?
In general, I think that the deadline for Waiting for Author patches
to move to Returned with Feedback should be based on when the review
happened + 5 days (maybe 4 days during the second half of the
CommitFest, or if the patch has been re-reviewed). If someone gets a
review at the beginning of the CommitFest, they should update their
patch within a few days of the review. They shouldn't get two extra
weeks to update their patch because they were lucky enough to get an
early review. On the other hand, someone who doesn't get a review
until later should get a reasonable time to respond if there's any
chance the patch can be fixed up quickly.
So there are a number of patches that I think we should mark as RWF
immediately, just because a lot of time has gone by:
PL/Python array support
Python 3.1 support
Memory management probes
Listen / Notify rewrite
For the rest, I agree that publishing a deadline on -hackers today is
a good idea. I don't think we can afford to give people much more
time, though: say until Tuesday, when there will be a week remaining
in the CF.
> 2) Poll the reviewer of every patch that's had an updated version who hasn't
> submitted a re-review asking whether they think that version is "ready for
> comitter" now, if they have more feedback, or if they feel it's just not
> ready yet and should be rejected. In any case but "ready for committer", it
> goes into "returned with feedback" pile.
Agreed.
> 3) Any patches in this state that we haven't heard back from the reviewer on
> within a couple of days get decided on ("ready" / "returned") at the
> CommitFest manager's discretion. If anyone feels wronged by that, they can
> always ask that a committer take a look anyway. The CF manager won't always
> have as much information as we expect the reviewers to, and can be presumed
> to have a thicker skin about people getting mad at them for making a bad
> decision too.
Yeah, that's pretty much what I've been doing.
> I have a deliverable to ship today, once I'm done with that I'll start
> rattling people more. Feedback about tweaking the above before I start
> executing on it would be appreciated.
I have some time tonight I'd be willing to put into taking a sweep
through the patch queue, if you'd like me to take a swing at it. If
not, that's OK too. I can't tell you how much I appreciate all the
work you've done so far. I really needed a break, and am feeling
infinitely better for it.
...Robert
Robert Haas wrote: > I made an effort to document this in the section of the "Running a > CommitFest" page that is entitled "Followup, Followup, Followup". But > that effort may not have been as successful as might have been wished All that documentation was a big help for getting started. The main problem with how you were chasing after things is that I think it was a bit too patch-specific. The parts that we still need to work on are more related to overall time flow. For example, I'd prefer to see some hard date guidelines for when exactly to start really aggressively booting patches out, rather than the useful but somewhat fuzzy ones you've got listed there now. The idea of giving every individual patch its own timetable is fair but very time intensive. I think we'll have much better luck at getting other people to do this job if it's chunked into a task checklist you run through once or twice a week. > I have some time tonight I'd be willing to put into taking a sweep > through the patch queue, if you'd like me to take a swing at it. I just finished up the big deliverable I had to deal with today, I'm about to dump a few hours into that job. You should continue enjoying your break. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Sun, Dec 6, 2009 at 7:10 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Robert Haas wrote: >> >> I made an effort to document this in the section of the "Running a >> CommitFest" page that is entitled "Followup, Followup, Followup". But >> that effort may not have been as successful as might have been wished > > All that documentation was a big help for getting started. The main problem > with how you were chasing after things is that I think it was a bit too > patch-specific. The parts that we still need to work on are more related to > overall time flow. For example, I'd prefer to see some hard date guidelines > for when exactly to start really aggressively booting patches out, rather > than the useful but somewhat fuzzy ones you've got listed there now. Fair enough. I'm not going to deny that my approach to this is fairly hands-on and informal, and making heavy use of intuition. It would be nice to tighten that up a bit and try to write out that intuition as a set of more formal guidelines. > The idea of giving every individual patch its own timetable is fair but very > time intensive. I think we'll have much better luck at getting other people > to do this job if it's chunked into a task checklist you run through once or Well, if the task list is - bug people who have had their patch reviewed but haven't updated it, that can work. But I'm firmly of the opinion that any system where there is a global timetable for all patches is doomed to failure. That will inevitably lead to a crush of people resubmitting just before that deadline, which will lead to a committer crunch later on. You have to look at the whole thing as a pipeline that constantly keeps reviewers, patch authors, and committers busy, without ever piling too much on any one group while the others sit idle. (Of course, at the end, things tend to build up a little on the committer end of things, but you want to try to minimize that by getting as much stuff as possible Ready for Committer earlier on.) ...Robert
Don't let chatter between me and Robert distract from the fact that I'm still looking for some help with the XLogInsert patch. My sole consolation is that it's something I can do myself if I have to in order to finish this up on schedule. As far as the rest of the "Needs review" items go, Euler says his two patch reviews are almost ready, Bernd just started looking at the DTrace patches the other day, and I nudged Kevin to advise on the tsearch patch because I have no personal insight into that one. That's it for patches we're trying to get covered via reviewers on this list, we're in decent shape now. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Sun, 2009-12-06 at 23:55 -0500, Greg Smith wrote:
> Don't let chatter between me and Robert distract from the fact that I'm
> still looking for some help with the XLogInsert patch. My sole
> consolation is that it's something I can do myself if I have to in order
> to finish this up on schedule.
I can help review this patch. However, the earliest I will get to it is
Tuesday.
Regards,
Jeff Davis
Jeff Davis wrote:
That seems to be earlier than anyone else will get to it, so if you have some time tomorrow to take a look at that would be great. I'm hoping to take a longer look at it myself too, you know how to grab me on chat if you want to coordinate that better.On Sun, 2009-12-06 at 23:55 -0500, Greg Smith wrote:Don't let chatter between me and Robert distract from the fact that I'm still looking for some help with the XLogInsert patch. My sole consolation is that it's something I can do myself if I have to in order to finish this up on schedule.I can help review this patch. However, the earliest I will get to it is Tuesday.
-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
This patch seems to be on hold waiting for performance numbers. I'll
wait for that before I look into this patch.
If there's something else that needs attention now, let me know.
Regards,
Jeff Davis
Jeff Davis wrote: > This patch seems to be on hold waiting for performance numbers. I'll > wait for that before I look into this patch. > Yes, I kicked it back at the author to help us out with that, since it really wasn't obvious how we were supposed to measure them, or exactly how his numbers were produced in the first place. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com