Code Reviews

[Josh Elser <jo...@gmail.com>]

Hi,

I'm being lazy and not looking through history, but has there been any 
discussion/consensus on the general topic of 
commit-then-review/review-then-commit for Calcite? It seems like most of 
the time it's just discretionary, but I figured I should ask instead of 
silently assuming.

Thanks!

Re: Code Reviews

[Josh Elser <jo...@gmail.com>]

Thanks for the summary, Julian.

Thinking about it, I think all of the projects where I'm a committer are 
actually CTR. My biggest complaint about this style is that it makes it 
really hard to make sure a second set of eyes is put on some code before 
it gets committed and forgotten about.

Currently, I'm of the mindset that some sort of CTR with some cool-off 
period before commit is a nice middle ground. You're not blocked on 
being able to commit, but it still gives the chance for some discussion 
before pushing it. This let's all of us participate to the degree that 
we choose.

Eventually, I think you'll lose your "dictatorship" role naturally (I 
definitely didn't feel like it was a monarchy -- just that you were the 
most experienced in the majority of the code).

Anyways, I appreciate the summary and think the current approach works 
well for Calcite as a community (which is the ultimate goal). If there's 
some consensus that encouraging more pre-commit review would be 
generally beneficial, I think there are some small changes we could 
discuss to help out. Meanwhile, I'll try to take a more active role in 
looking at patches, even when not "@"-pinged :)

Julian Hyde wrote:
> Thanks for bringing this up. We’ve never actually had that discussion, so the official policy is very loose.
>
> The practice is for committers to use their discretion. No committer needs to ask for permission, but it is polite and prudent to ask for review if it is a large change and/or in an area that the committer is not familiar with.
>
> In my opinion, the current practice seems to be working well. People are not subjected to undue delays waiting for review, we are not turning away would-be contributors by being too harsh, nor are we admitting dubious code into the code base.
>
> But I would say that! It is a valid criticism that Calcite is somewhat of a benevolent dictatorship, with me as the dictator, writing a lot of the code and doing a lot of the reviews. Other committers tend to ask for their commits to be reviewed, perhaps out of deference. Benevolent dictatorships work well in other open source projects (think Linux[1], Python, Ruby), but are not the Apache Way. I want to dismantle the perception and reality that the project is run that way.
>
> If a more explicit and prescriptive policy would help make Calcite more egalitarian, let’s consider it.
>
> Julian
>
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1510.3/02866.html<http://lkml.iu.edu/hypermail/linux/kernel/1510.3/02866.html>
>
>
>
>
>> On Nov 11, 2015, at 2:33 PM, Josh Elser<jo...@gmail.com>  wrote:
>>
>> Hi,
>>
>> I'm being lazy and not looking through history, but has there been any discussion/consensus on the general topic of commit-then-review/review-then-commit for Calcite? It seems like most of the time it's just discretionary, but I figured I should ask instead of silently assuming.
>>
>> Thanks!
>
>

Re: Code Reviews

[Julian Hyde <jh...@apache.org>]

Thanks for bringing this up. We’ve never actually had that discussion, so the official policy is very loose.

The practice is for committers to use their discretion. No committer needs to ask for permission, but it is polite and prudent to ask for review if it is a large change and/or in an area that the committer is not familiar with.

In my opinion, the current practice seems to be working well. People are not subjected to undue delays waiting for review, we are not turning away would-be contributors by being too harsh, nor are we admitting dubious code into the code base.

But I would say that! It is a valid criticism that Calcite is somewhat of a benevolent dictatorship, with me as the dictator, writing a lot of the code and doing a lot of the reviews. Other committers tend to ask for their commits to be reviewed, perhaps out of deference. Benevolent dictatorships work well in other open source projects (think Linux[1], Python, Ruby), but are not the Apache Way. I want to dismantle the perception and reality that the project is run that way.

If a more explicit and prescriptive policy would help make Calcite more egalitarian, let’s consider it.

Julian

[1] http://lkml.iu.edu/hypermail/linux/kernel/1510.3/02866.html <http://lkml.iu.edu/hypermail/linux/kernel/1510.3/02866.html>




> On Nov 11, 2015, at 2:33 PM, Josh Elser <jo...@gmail.com> wrote:
> 
> Hi,
> 
> I'm being lazy and not looking through history, but has there been any discussion/consensus on the general topic of commit-then-review/review-then-commit for Calcite? It seems like most of the time it's just discretionary, but I figured I should ask instead of silently assuming.
> 
> Thanks!