[DISCUSS] Reviewing Arrow commit/code review policy

[Wes McKinney <we...@gmail.com>]

hi folks,

We've added many new committers to Apache Arrow over the last 3+
years, so I thought it would be worthwhile to review our commit and
code review policy for everyone's benefit.

Since the beginning of the project, Arrow has been in "Commit Then
Review" mode (aka CTR).

https://www.apache.org/foundation/glossary.html#CommitThenReview

The idea of CTR is that committers can make changes at will with the
understanding that if there is some disagreement or if work is vetoed,
then changes may be reverted.

In particular, in CTR if a committer submits a patch, they are able to
+1 and merge their own patch. Generally, though, as a matter of
courtesy to the community, for non-trivial patches it is a good idea
to allow time for code review.

More mature projects, or ones with potentially contentious governance
/ political issues, sometimes adopt "Review-Then-Commit" (RTC) which
requires a more structured sign-off process from other committers.
While Apache Arrow is more mature now, the diversity of the project
has resulted in a lot of spread-out code ownership. I think that RTC
at this stage would cause hardship for contributors on some components
where there are not a lot of active code reviewers.

Personally, I am OK to stick with CTR until we start experiencing
problems. Overall I think we have a healthy dynamic amongst the
project's nearly 50 committers and we have had to revert patches
relatively rarely.

Any thoughts from others?

Thanks
Wes

Re: [DISCUSS] Reviewing Arrow commit/code review policy

["Uwe L. Korn" <uw...@xhochy.com>]

Hello all,

I also think we should stay with CTR for the moment. If we wanted to enforce RTC or at least a bit better notification for reviewers of certain parts of Arrow, we could setup a CODEOWNERS file[1] to add experts of a certain file/folder as a reviewer on PRs on Github.

Cheers
Uwe

[1] https://help.github.com/en/articles/about-code-owners

On Sun, Oct 13, 2019, at 2:03 AM, Andy Grove wrote:
> Wes,
> 
> Thanks for clarifying this. This will be very helpful for me while I work
> on the Rust DataFusion crate since we have a small number of committers
> today. I will still generally make PRs available for review (unless they
> are trivial changes) but being able to merge without review when the other
> committers are busy will be very helpful for momentum with some of the
> features that I would like to see in the 1.0.0 release.
> 
> Thanks,
> 
> Andy.
> 
> On Sat, Oct 12, 2019 at 2:51 PM Wes McKinney <we...@gmail.com> wrote:
> 
> > hi folks,
> >
> > We've added many new committers to Apache Arrow over the last 3+
> > years, so I thought it would be worthwhile to review our commit and
> > code review policy for everyone's benefit.
> >
> > Since the beginning of the project, Arrow has been in "Commit Then
> > Review" mode (aka CTR).
> >
> > https://www.apache.org/foundation/glossary.html#CommitThenReview
> >
> > The idea of CTR is that committers can make changes at will with the
> > understanding that if there is some disagreement or if work is vetoed,
> > then changes may be reverted.
> >
> > In particular, in CTR if a committer submits a patch, they are able to
> > +1 and merge their own patch. Generally, though, as a matter of
> > courtesy to the community, for non-trivial patches it is a good idea
> > to allow time for code review.
> >
> > More mature projects, or ones with potentially contentious governance
> > / political issues, sometimes adopt "Review-Then-Commit" (RTC) which
> > requires a more structured sign-off process from other committers.
> > While Apache Arrow is more mature now, the diversity of the project
> > has resulted in a lot of spread-out code ownership. I think that RTC
> > at this stage would cause hardship for contributors on some components
> > where there are not a lot of active code reviewers.
> >
> > Personally, I am OK to stick with CTR until we start experiencing
> > problems. Overall I think we have a healthy dynamic amongst the
> > project's nearly 50 committers and we have had to revert patches
> > relatively rarely.
> >
> > Any thoughts from others?
> >
> > Thanks
> > Wes
> >
>

Re: [DISCUSS] Reviewing Arrow commit/code review policy

[Andy Grove <an...@gmail.com>]

Wes,

Thanks for clarifying this. This will be very helpful for me while I work
on the Rust DataFusion crate since we have a small number of committers
today. I will still generally make PRs available for review (unless they
are trivial changes) but being able to merge without review when the other
committers are busy will be very helpful for momentum with some of the
features that I would like to see in the 1.0.0 release.

Thanks,

Andy.

On Sat, Oct 12, 2019 at 2:51 PM Wes McKinney <we...@gmail.com> wrote:

> hi folks,
>
> We've added many new committers to Apache Arrow over the last 3+
> years, so I thought it would be worthwhile to review our commit and
> code review policy for everyone's benefit.
>
> Since the beginning of the project, Arrow has been in "Commit Then
> Review" mode (aka CTR).
>
> https://www.apache.org/foundation/glossary.html#CommitThenReview
>
> The idea of CTR is that committers can make changes at will with the
> understanding that if there is some disagreement or if work is vetoed,
> then changes may be reverted.
>
> In particular, in CTR if a committer submits a patch, they are able to
> +1 and merge their own patch. Generally, though, as a matter of
> courtesy to the community, for non-trivial patches it is a good idea
> to allow time for code review.
>
> More mature projects, or ones with potentially contentious governance
> / political issues, sometimes adopt "Review-Then-Commit" (RTC) which
> requires a more structured sign-off process from other committers.
> While Apache Arrow is more mature now, the diversity of the project
> has resulted in a lot of spread-out code ownership. I think that RTC
> at this stage would cause hardship for contributors on some components
> where there are not a lot of active code reviewers.
>
> Personally, I am OK to stick with CTR until we start experiencing
> problems. Overall I think we have a healthy dynamic amongst the
> project's nearly 50 committers and we have had to revert patches
> relatively rarely.
>
> Any thoughts from others?
>
> Thanks
> Wes
>