[OpenLayers-Dev] Commits without review (Was: #1218: Serialization doesn't wrap nodes in documents)

Stephen Woodbridge woodbri at swoodbridge.com
Tue Dec 18 19:32:58 EST 2007


Christopher,

A few random thoughts and ideas that may or may not be helpful.

1) Define what is expected of a reviewer. Why?
  - It might relieve reviewers from assuming that much more is expected
  - There might be different levels of expectations depending on what is 
be reviewed
  - Maybe a checklist is appropriate to set some minimum standards for 
review
    o is a test case needed
    o is a test case provided
    o does this appear to impact any APIs
    o does this appear to have side effects that might be unwanted
    o is the change clear and obvious
    o etc but in general keep is simple and inline with the complexity 
of the change, so it is obvious if it should be included or rejected.

2) Have the committers agree to spend 20%+- of there time reviewing patches.
3) Have a review and commit code sprint at some regular basis
4) Try putting trivial patches into a 5 day review and commit unless 
rejected queue. This might need to be titrated until the backlog is 
reduced so the queue does not get overwhelmed.
5) Is there an opportunity create a class of committer that is review 
and commit only, maybe as an initial step to becoming a full committer, 
this would give feedback on there judgment and an opportunity for 
education if problems occur, but these would be mostly minor issues.

I'm sure some of these will make no sense, as they are mostly off the 
top of my head, but I hope that you can bounce off these ideas to find 
something that might help.

Best regards,
   -Stephen Woodbridge

Christopher Schmidt wrote:
> On Tue, Dec 18, 2007 at 03:13:30PM -0700, Tim Schaub wrote:
>> Great.  Can you try to capture some of your ideas for a new process on a 
>> wiki page?  As I mentioned from the start, I'm eager to get a process 
>> down that we all agree with.  I also mentioned that I think the 
>> following three items are a recipe for disaster:
>>
>> 1) commits without review
>> 2) more committers
>> 3) frequent releases
>>
>> As I said before, I'm in favor of 2 and 3.  Your message subject 
>> suggests 1.  Since I think those three items conflict, I'm just asking 
>> you to give some more detail on what you would like implemented.
> 
> I guess I don't understand how things like #1082 would impact 2 or 3.
> 
> Problem statement: Changes take too long to make it into trunk. 
> Specific subset of the problem that i'm interested in: the reason for
> this is not bad code or things which are otherwise lacking, but instead,
> lack of reviewers.
> 
> I see two possible solutions.
> 
>  1. small commits without review. Definition of small may vary. I'd
>     take the xml format serializer as an example of an upper bound on
>     small.
>  2. Somehow get more people reviewing.
> 
> The former helps to accelerate reviews because the small things 'go
> away' after some time period of no one objecting. This works in cases
> where API methods are not affected, because committers still have the
> ability to object. Even API Methods have this  attribute until a
> release.
> 
> Adding more committers does not -- at least in the short run -- get us
> more reviews, because new committers are unlikely to take initiative to
> mark something as reviewed. (Understandably so.) So I don't actually see
> ho to change #2 -- and adding more committers only increases our patch
> backlog until we get more reviewers. 
> 
> So, your #2 above would make the situation worse: I'm already reviewing
> everything I can, and things are still sitting for several months
> without any activity, includig patches contributed by non-committer
> users who are likely to just go away when they find no feedback from the
> community on their patches.
> 
> So, a possible direction:
>  * Everything still gets a ticket.
>  * Patch is uploaded to the ticket.
>  * Assuming the patch neither adds nor modifies function signature of
>    any method, and no objections are voiced after some time period, the
>    ticket can go in.
> 
> This helps to prevent the large patch backlog we have now -- where I'm
> concentrating more on making sure my patches are still moving forward
> than reviewing other patches.
> 
> Is there some other way to increase the number of patches that go
> through? I'm assuming that others aren't revieing because they have no
> time: I try very hard to keep the
> http://trac.openlayers.org/wiki/Release/2.6 page inline, which means
> that there is always a 'list of things which need review' which easily
> accessible, so I'm assuming more information in that direction doesn't
> actually help. 
> 
> Why aren't people reviewing? Fear of retribution from on high? Not enough
> time? Not enough insight into the project?
> 
> Maybe there's an obvious solution that I'm missing. I have always
> assumed that our review process simply assigns too much responsibility,
> and therefore committers are unwilling to participate iwthout
> deep/intimate knowledge. Certainly I feel like every time I commit
> something which adds an APIMethod or APIProperty without first reviewing
> it with Tim or Erik, I have done something wrong, so I simply don't
> review most things, and instead try to draw attention to tickets to
> gather feedback, typically unsuccessfully. 
> 
> I feel like I spend 6-8 hours every weekend on working on patches,
> reviews, etc. and never move the OpenLayers codebase forward in any
> significant way. I'd like to change that. I don't know what the solution
> is, but I'm worried that if I feel this way, other contributors who
> aren't committers must be feeling that way to a much greater extent.
> 
> Regards,




More information about the Dev mailing list