[DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[siddharth anand <sa...@apache.org>]

Folks,
It's a good time for us to discuss whether we will adopt a RTC or CTR
policy towards software changes.

In the RTC case, we need 3 +1 binding (a.k.a. committer) votes and 0
binding vetos for any change as RTC requires consensus approval:

   - http://www.apache.org/foundation/glossary.html#ReviewThenCommit
   - http://www.apache.org/foundation/glossary.html#ConsensusApproval

In the CTR case, we operate by lazy consensus, which many of the committers
have already exercised for the recent Committer/PPMC vote. In the CTR case,
we may want a separate develop branch against which to run integration
tests and merge to master only after those tests pass. I'm curious about
this approach and would like to understand how well this is supported via
infra tools, automation, and documentation.

   - http://www.apache.org/foundation/glossary.html#CommitThenReview
   - http://www.apache.org/foundation/glossary.html#LazyConsensus

I'm also curious if we need to strictly adopt one of these or whether we
can roll our own - e.g. a +1 binding for RTC for example.

Mentors,
Any guidance here?

-s

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[Jakob Homan <jg...@gmail.com>]

It's up to each community to decide what it's comfortable with, and
then make sure the process is applied equally to everyone.  In my
experience, 'non-minor, trivial' is pretty nebulous.  Does a
3,000-line IDE-driven refactor count as non-minor?  What about a one
line change in the heart of the scheduler that inadvertently changes a
!= to a ==?

It's generally agreed that immediate fixes for broken builds (e.g.,
missing seimicolons that break the compiler, or the Python
equivalent), can be checked in without reviews (they're usually
attached to the original ticket).

-jakob

On 20 May 2016 at 09:23, siddharth anand <r3...@gmail.com> wrote:
> Can we suggest a +1 vote for non-minor/non-trivial commits, or do all
> commits require a +1? Or is the "non-minor, non-trivial" wording too
> nebulous. t feels like this would work better the more committers we have
> given our different daily work schedules.
>
> -s
>
> On Thu, May 19, 2016 at 5:56 PM, Jakob Homan <jg...@gmail.com> wrote:
>
>> On 18 May 2016 at 16:06, siddharth anand <r3...@gmail.com> wrote:
>> > Is there an automation we can use to ensure/enforce that PRs
>> > and JIRAs receive the +1 binding (from committers) votes needed before a
>> > merge can proceed
>>
>> Not that I know of.  Since we're using github PRs, there might be some
>> kind of workflow system that plugs into that, but I'm not aware of it.
>> Honor code works well, and reverting commits work when it doesn't...
>>

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[siddharth anand <r3...@gmail.com>]

Can we suggest a +1 vote for non-minor/non-trivial commits, or do all
commits require a +1? Or is the "non-minor, non-trivial" wording too
nebulous. t feels like this would work better the more committers we have
given our different daily work schedules.

-s

On Thu, May 19, 2016 at 5:56 PM, Jakob Homan <jg...@gmail.com> wrote:

> On 18 May 2016 at 16:06, siddharth anand <r3...@gmail.com> wrote:
> > Is there an automation we can use to ensure/enforce that PRs
> > and JIRAs receive the +1 binding (from committers) votes needed before a
> > merge can proceed
>
> Not that I know of.  Since we're using github PRs, there might be some
> kind of workflow system that plugs into that, but I'm not aware of it.
> Honor code works well, and reverting commits work when it doesn't...
>

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[Jakob Homan <jg...@gmail.com>]

On 18 May 2016 at 16:06, siddharth anand <r3...@gmail.com> wrote:
> Is there an automation we can use to ensure/enforce that PRs
> and JIRAs receive the +1 binding (from committers) votes needed before a
> merge can proceed

Not that I know of.  Since we're using github PRs, there might be some
kind of workflow system that plugs into that, but I'm not aware of it.
Honor code works well, and reverting commits work when it doesn't...

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[siddharth anand <r3...@gmail.com>]

Thanks for sharing this information.

All,
Please share any additional thoughts. In a few days, I will kick off a VOTE
thread to decide our process.

Mentors,
Just curious! Is there an automation we can use to ensure/enforce that PRs
and JIRAs receive the +1 binding (from committers) votes needed before a
merge can proceed? Something like a drop-in client-side pre-git-push hook
script for pushes to apache master. Or should we just follow an honor code.

-s

On Wed, May 18, 2016 at 6:14 PM, Jakob Homan <jg...@gmail.com> wrote:

> Hey-
>    The consensus approval link is referring to votes (e.g. releases,
> new committers, new PMCers, etc) rather than code changes.  A single
> +1 is standard for code changes, as Chris describes.  However,
> projects are welcome to provide more fine-grained requirements as
> well.  For example, Hadoop is RTC (Review-then-commit) but also
> features to be developed in branches that run on CTR
> (Commit-then-review).  Such a branch can only be merged back to master
> with three binding +1s, as a counterweight to the possibility of
> un-reviewed changes appearing.
>
>   Historically, ASF operated on a CTR model.  However, Hadoop and the
> myriad projects that flowed from it, adopted RTC.  I'm not aware of
> any big-data/Hadoop/Kafka projects that currently use CTR.  (This
> comment's not an endorsement of either approach, just an observation)
>
>    Additionally, the community can chose any time to change its
> approach (via a vote).
> -Jakob
>
>
> On 18 May 2016 at 10:55, Chris Riccomini <cr...@apache.org> wrote:
> > Hey Sid,
> >
> >> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes
> >
> > This sounds very high. Usually one +1 (other than the person sending the)
> > is normal in an RTC scenario.
> >
> >> In the CTR case, we may want a separate develop branch against which to
> > run integration tests and merge to master only after those tests pass
> >
> > I would prefer not to have a separate branch, so if we feel like that's a
> > requirement for CTR, then I'd prefer RTC. If we're comfortable committing
> > to master, though, then I'm fine either way. We have quite a few active
> > committers, so waiting 24h for a review seems fine.
> >
> > Basically, I don't have a strong preference either way, except that I
> > strongly prefer not to have a separate development branch. If you force
> me
> > to pick, I'd pick RTC. I find that RTC encourages a shared understanding
> of
> > the code, which is useful.
> >
> > Cheers,
> > Chris
> >
> > On Tue, May 17, 2016 at 8:10 PM, siddharth anand <sa...@apache.org>
> wrote:
> >
> >> Folks,
> >> It's a good time for us to discuss whether we will adopt a RTC or CTR
> >> policy towards software changes.
> >>
> >> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes and 0
> >> binding vetos for any change as RTC requires consensus approval:
> >>
> >>    - http://www.apache.org/foundation/glossary.html#ReviewThenCommit
> >>    - http://www.apache.org/foundation/glossary.html#ConsensusApproval
> >>
> >> In the CTR case, we operate by lazy consensus, which many of the
> committers
> >> have already exercised for the recent Committer/PPMC vote. In the CTR
> case,
> >> we may want a separate develop branch against which to run integration
> >> tests and merge to master only after those tests pass. I'm curious about
> >> this approach and would like to understand how well this is supported
> via
> >> infra tools, automation, and documentation.
> >>
> >>    - http://www.apache.org/foundation/glossary.html#CommitThenReview
> >>    - http://www.apache.org/foundation/glossary.html#LazyConsensus
> >>
> >> I'm also curious if we need to strictly adopt one of these or whether we
> >> can roll our own - e.g. a +1 binding for RTC for example.
> >>
> >> Mentors,
> >> Any guidance here?
> >>
> >> -s
> >>
>

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[Jakob Homan <jg...@gmail.com>]

Hey-
   The consensus approval link is referring to votes (e.g. releases,
new committers, new PMCers, etc) rather than code changes.  A single
+1 is standard for code changes, as Chris describes.  However,
projects are welcome to provide more fine-grained requirements as
well.  For example, Hadoop is RTC (Review-then-commit) but also
features to be developed in branches that run on CTR
(Commit-then-review).  Such a branch can only be merged back to master
with three binding +1s, as a counterweight to the possibility of
un-reviewed changes appearing.

  Historically, ASF operated on a CTR model.  However, Hadoop and the
myriad projects that flowed from it, adopted RTC.  I'm not aware of
any big-data/Hadoop/Kafka projects that currently use CTR.  (This
comment's not an endorsement of either approach, just an observation)

   Additionally, the community can chose any time to change its
approach (via a vote).
-Jakob


On 18 May 2016 at 10:55, Chris Riccomini <cr...@apache.org> wrote:
> Hey Sid,
>
>> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes
>
> This sounds very high. Usually one +1 (other than the person sending the)
> is normal in an RTC scenario.
>
>> In the CTR case, we may want a separate develop branch against which to
> run integration tests and merge to master only after those tests pass
>
> I would prefer not to have a separate branch, so if we feel like that's a
> requirement for CTR, then I'd prefer RTC. If we're comfortable committing
> to master, though, then I'm fine either way. We have quite a few active
> committers, so waiting 24h for a review seems fine.
>
> Basically, I don't have a strong preference either way, except that I
> strongly prefer not to have a separate development branch. If you force me
> to pick, I'd pick RTC. I find that RTC encourages a shared understanding of
> the code, which is useful.
>
> Cheers,
> Chris
>
> On Tue, May 17, 2016 at 8:10 PM, siddharth anand <sa...@apache.org> wrote:
>
>> Folks,
>> It's a good time for us to discuss whether we will adopt a RTC or CTR
>> policy towards software changes.
>>
>> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes and 0
>> binding vetos for any change as RTC requires consensus approval:
>>
>>    - http://www.apache.org/foundation/glossary.html#ReviewThenCommit
>>    - http://www.apache.org/foundation/glossary.html#ConsensusApproval
>>
>> In the CTR case, we operate by lazy consensus, which many of the committers
>> have already exercised for the recent Committer/PPMC vote. In the CTR case,
>> we may want a separate develop branch against which to run integration
>> tests and merge to master only after those tests pass. I'm curious about
>> this approach and would like to understand how well this is supported via
>> infra tools, automation, and documentation.
>>
>>    - http://www.apache.org/foundation/glossary.html#CommitThenReview
>>    - http://www.apache.org/foundation/glossary.html#LazyConsensus
>>
>> I'm also curious if we need to strictly adopt one of these or whether we
>> can roll our own - e.g. a +1 binding for RTC for example.
>>
>> Mentors,
>> Any guidance here?
>>
>> -s
>>

Re: [DISCUSS] Review-then-commit (RTC) vs Commit-then-review (CTR)

[Chris Riccomini <cr...@apache.org>]

Hey Sid,

> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes

This sounds very high. Usually one +1 (other than the person sending the)
is normal in an RTC scenario.

> In the CTR case, we may want a separate develop branch against which to
run integration tests and merge to master only after those tests pass

I would prefer not to have a separate branch, so if we feel like that's a
requirement for CTR, then I'd prefer RTC. If we're comfortable committing
to master, though, then I'm fine either way. We have quite a few active
committers, so waiting 24h for a review seems fine.

Basically, I don't have a strong preference either way, except that I
strongly prefer not to have a separate development branch. If you force me
to pick, I'd pick RTC. I find that RTC encourages a shared understanding of
the code, which is useful.

Cheers,
Chris

On Tue, May 17, 2016 at 8:10 PM, siddharth anand <sa...@apache.org> wrote:

> Folks,
> It's a good time for us to discuss whether we will adopt a RTC or CTR
> policy towards software changes.
>
> In the RTC case, we need 3 +1 binding (a.k.a. committer) votes and 0
> binding vetos for any change as RTC requires consensus approval:
>
>    - http://www.apache.org/foundation/glossary.html#ReviewThenCommit
>    - http://www.apache.org/foundation/glossary.html#ConsensusApproval
>
> In the CTR case, we operate by lazy consensus, which many of the committers
> have already exercised for the recent Committer/PPMC vote. In the CTR case,
> we may want a separate develop branch against which to run integration
> tests and merge to master only after those tests pass. I'm curious about
> this approach and would like to understand how well this is supported via
> infra tools, automation, and documentation.
>
>    - http://www.apache.org/foundation/glossary.html#CommitThenReview
>    - http://www.apache.org/foundation/glossary.html#LazyConsensus
>
> I'm also curious if we need to strictly adopt one of these or whether we
> can roll our own - e.g. a +1 binding for RTC for example.
>
> Mentors,
> Any guidance here?
>
> -s
>