Branch policy question

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Branch policy question

Allen Wittenauer-2

        Since it’s nearly impossible for me to get timely reviews for some build and script changes, is it possible for me to setup a branch, self review+commit to that branch, then request a branch merge?  I’m basically looking at doing this for HADOOP-12857 + HADOOP-12930 and their subtasks esp since they are very order dependent.  I don’t know if a branch is easier to review, honestly, but at least I wouldn’t be blocked on making progress...

        Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

larry mccay-3
That sounds like a reasonable approach and valid use of branches to me.

Perhaps a set of functional tests could be provided/identified that would
help the review process by showing backward compatibility along with new
extensions for things like dynamic commands?


On Tue, Mar 22, 2016 at 12:14 PM, Allen Wittenauer <[hidden email]> wrote:

>
>         Since it’s nearly impossible for me to get timely reviews for some
> build and script changes, is it possible for me to setup a branch, self
> review+commit to that branch, then request a branch merge?  I’m basically
> looking at doing this for HADOOP-12857 + HADOOP-12930 and their subtasks
> esp since they are very order dependent.  I don’t know if a branch is
> easier to review, honestly, but at least I wouldn’t be blocked on making
> progress...
>
>         Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Allen Wittenauer-5

> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]> wrote:
>
> That sounds like a reasonable approach and valid use of branches to me.
>
> Perhaps a set of functional tests could be provided/identified that would
> help the review process by showing backward compatibility along with new
> extensions for things like dynamic commands?
>

        This is going into trunk, so no need for backward compatibility.
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Andrew Wang
A branch sounds fine, but how are we going to get 3 +1's to merge it? If
it's hard to find one reviewer, seems even harder to find two.

On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
[hidden email]> wrote:

>
> > On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]> wrote:
> >
> > That sounds like a reasonable approach and valid use of branches to me.
> >
> > Perhaps a set of functional tests could be provided/identified that would
> > help the review process by showing backward compatibility along with new
> > extensions for things like dynamic commands?
> >
>
>         This is going into trunk, so no need for backward compatibility.
>
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Sean Busbey
VOTE threads tend to get more eyes than random JIRAs.

On Tue, Mar 22, 2016 at 1:23 PM, Andrew Wang <[hidden email]>
wrote:

> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
> it's hard to find one reviewer, seems even harder to find two.
>
> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
> [hidden email]> wrote:
>
> >
> > > On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]>
> wrote:
> > >
> > > That sounds like a reasonable approach and valid use of branches to me.
> > >
> > > Perhaps a set of functional tests could be provided/identified that
> would
> > > help the review process by showing backward compatibility along with
> new
> > > extensions for things like dynamic commands?
> > >
> >
> >         This is going into trunk, so no need for backward compatibility.
> >
>



--
busbey
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Colin McCabe-3
In reply to this post by Andrew Wang
If the underlying problem is lack of reviewers for these improvements,
how about a design doc giving some motivation for the improvements and
explaining how they'll be implemented?  Then we can decide if a branch
or a few JIRAs on trunk makes more sense.

The description for HADOOP-12857 is just "let's rework this to be
smarter" which could be the description of any JIRA in Hadoop (except
for the ones which make things dumber, of course).

best,
Colin


On Tue, Mar 22, 2016, at 11:23, Andrew Wang wrote:

> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
> it's hard to find one reviewer, seems even harder to find two.
>
> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
> [hidden email]> wrote:
>
> >
> > > On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]> wrote:
> > >
> > > That sounds like a reasonable approach and valid use of branches to me.
> > >
> > > Perhaps a set of functional tests could be provided/identified that would
> > > help the review process by showing backward compatibility along with new
> > > extensions for things like dynamic commands?
> > >
> >
> >         This is going into trunk, so no need for backward compatibility.
> >
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Gangumalla, Uma
In reply to this post by Allen Wittenauer-2
> is it possible for me to setup a branch, self review+commit to that
>branch, then request a branch merge?
Basically this is something like Commit-Then-Review(here review later)
process right. I have not seen we followed this approach here( not sure if
I missed some branches followed that). Even though original author code
quality is good, there would always be chances for missing somethings. So,
peer review is important because the other eye can catch some issues which
original author might overlooked (general review advantage :-)). In this
case for branch merge we need three  +1s. if we face difficult in getting
one +1, then I am afraid that we may face more difficult to get reviewers
a the time of merge, because code base much larger than normal patch. Some
times we suggest contributors to split patches into multiple JIRAs of
patch size is becoming larger. It is better to find some reviewers for the
branch and creating branch could turn into healthy merge later.

Colin suggestion sounds good to me. How about providing more details and
find some reviewers (who is more familiar in that area etc)?

If this is general question question for branch policy MY answer is ³no²
for "self review+commit to that branch, then request a branch merge². But
for this kind of special cases where we are sure that we may not have
enough reviewers for branch, having dev mailing discussion about that
JIRA/branch proposal and see how to go about that changes may be good idea
instead of going ahead finishing work and raising merge vote thread ?
(Something like this what you did now :-))

Just my thoughts on this discussion.

Thanks & Regards,
Uma

On 3/22/16, 9:14 AM, "Allen Wittenauer" <[hidden email]> wrote:

>Since it¹s nearly impossible for me to get timely reviews for some build
>and script changes, is it possible for me to setup a branch, self
>review+commit to that branch, then request a branch merge?

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

larry mccay-2
Just to clarify, we are talking about a feature branch in which Allen and
others that are working on the branch could commit without requiring 3 +1’s.
Then, at some point, we will need 3 +1’s to merge the branch to trunk.
Correct?

I think that if we have a set of usecases that are being added and those
that are being reworked with test coverage for them all - maybe including
manual testing scenarios - that we could find folks that would be willing
to work with Allen to get it reviewed. I am listed as a branch committer.
If my vote would count then I would be more than willing to lend a hand
where I can - especially if we have the tests/instructions for testing.

I think that we need to strike a balance of not inhibiting progress and at
the same time not getting a patch so large that it can’t be reviewed.
Colin's point is valid and a design document that calls out the areas that
will need testing and general approach would help review volunteers.

There is always a risk with branches that the change gets to a level of
complexity that it is too hard to reasonably review. I think that the
community should allow folks to take that risk and try and provide enough
guidance and transparency for review. Any non-trivial branch effort should
have a merge strategy for getting it reviewed, tested and merged.


On Tue, Mar 22, 2016 at 9:46 PM, Gangumalla, Uma <[hidden email]>
wrote:

> > is it possible for me to setup a branch, self review+commit to that
> >branch, then request a branch merge?
> Basically this is something like Commit-Then-Review(here review later)
> process right. I have not seen we followed this approach here( not sure if
> I missed some branches followed that). Even though original author code
> quality is good, there would always be chances for missing somethings. So,
> peer review is important because the other eye can catch some issues which
> original author might overlooked (general review advantage :-)). In this
> case for branch merge we need three  +1s. if we face difficult in getting
> one +1, then I am afraid that we may face more difficult to get reviewers
> a the time of merge, because code base much larger than normal patch. Some
> times we suggest contributors to split patches into multiple JIRAs of
> patch size is becoming larger. It is better to find some reviewers for the
> branch and creating branch could turn into healthy merge later.
>
> Colin suggestion sounds good to me. How about providing more details and
> find some reviewers (who is more familiar in that area etc)?
>
> If this is general question question for branch policy MY answer is ³no²
> for "self review+commit to that branch, then request a branch merge². But
> for this kind of special cases where we are sure that we may not have
> enough reviewers for branch, having dev mailing discussion about that
> JIRA/branch proposal and see how to go about that changes may be good idea
> instead of going ahead finishing work and raising merge vote thread ?
> (Something like this what you did now :-))
>
> Just my thoughts on this discussion.
>
> Thanks & Regards,
> Uma
>
> On 3/22/16, 9:14 AM, "Allen Wittenauer" <[hidden email]> wrote:
>
> >Since it¹s nearly impossible for me to get timely reviews for some build
> >and script changes, is it possible for me to setup a branch, self
> >review+commit to that branch, then request a branch merge?
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Allen Wittenauer-5
In reply to this post by Gangumalla, Uma

> On Mar 22, 2016, at 6:46 PM, Gangumalla, Uma <[hidden email]> wrote:
>
>> is it possible for me to setup a branch, self review+commit to that
>> branch, then request a branch merge?
> Basically this is something like Commit-Then-Review(here review later)
> process right. I have not seen we followed this approach here( not sure if
> I missed some branches followed that).

        At least in recent times, YARN-3368 attempted to do that with the merge request found in YARN-4734. The first or second commit that went into that branch clearly has never been reviewed by another committer.  Since a PMC member did the commit and merge request, I thought I’d ask since it implies that the policy really is CTR for merge requests. (FWIW, I was the lone -1 on the merge request, otherwise it likely would have gone in given it already had 2 +1’s despite a) breaking the build b) having significant license issues and c) no design doc.)

       
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Chris Nauroth
It sounds like the community doesn't have a consistent point of view on
the policy.

Setting aside policy for a moment, I've always considered a few different
branching models available when I start coding something big, with each
model offering different features.

1. Apache mainline branches: Review then commit.  Sign-off required from a
full committer.  Check the pre-commit results.  Don't break the build.
Strive to keep the branch shippable.

2. Apache feature branches: Sign-off may come from designated branch
committers in addition to full committers.  It's OK to break the branch
for work in progress, but it must be fixed before a merge.  It's still
review then commit though.

3. Personal GitHub repos: Commit as much as you want with no consequences.
 Great for speculative coding before requesting a full review, and great
for staging a sequence of dependent patches.  This is not visible to the
Apache community though, so you run the risk of needing to toss a bunch of
work later if someone objects to the changes for fundamental design
reasons.

Allen, are there particular reasons that you favor #2 (but with
commit-then-review) instead of #3 for your work on HADOOP-12857 and
HADOOP-12930?  Maybe you want the patches on Apache infrastructure so you
can get the pre-commit sign-offs quickly?  Is it something else?

I think the spirit of feature branches is to provide a more rapid
development path for larger efforts.  As such, I'd have no objection to
allowing commit-then-review on feature branches if there is a good reason
for it.  By definition, a feature branch is isolated from the shipping
branches, so there is no risk of harming a release.

If community members are asking for design documentation and efforts to
rally more people to look at the changes, then that needs to be addressed
to the community's satisfaction too.  (Personally, I think design
documents are more heavy-weight of a process than required for these
changes, but others might legitimately think differently.)  If this isn't
addressed, then your obstacles on this work are just deferred to merge
time.  Allowing commit-then-review might allow the dev work to continue in
parallel while doing this though.

--Chris Nauroth




On 3/22/16, 11:03 PM, "Allen Wittenauer"
<[hidden email]> wrote:

>
>> On Mar 22, 2016, at 6:46 PM, Gangumalla, Uma <[hidden email]>
>>wrote:
>>
>>> is it possible for me to setup a branch, self review+commit to that
>>> branch, then request a branch merge?
>> Basically this is something like Commit-Then-Review(here review later)
>> process right. I have not seen we followed this approach here( not sure
>>if
>> I missed some branches followed that).
>
> At least in recent times, YARN-3368 attempted to do that with the merge
>request found in YARN-4734. The first or second commit that went into
>that branch clearly has never been reviewed by another committer.  Since
>a PMC member did the commit and merge request, I thought I¹d ask since it
>implies that the policy really is CTR for merge requests. (FWIW, I was
>the lone -1 on the merge request, otherwise it likely would have gone in
>given it already had 2 +1¹s despite a) breaking the build b) having
>significant license issues and c) no design doc.)
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Colin McCabe-3
> On 3/22/16, 11:03 PM, "Allen Wittenauer"
> <[hidden email]> wrote:
>
> >> On Mar 22, 2016, at 6:46 PM, Gangumalla, Uma <[hidden email]>
> >>wrote:
> >>
> >>> is it possible for me to setup a branch, self review+commit to that
> >>> branch, then request a branch merge?
> >> Basically this is something like Commit-Then-Review(here review later)
> >> process right. I have not seen we followed this approach here( not sure
> >>if
> >> I missed some branches followed that).
> >
> > At least in recent times, YARN-3368 attempted to do that with the merge
> >request found in YARN-4734. The first or second commit that went into
> >that branch clearly has never been reviewed by another committer.  Since
> >a PMC member did the commit and merge request, I thought I¹d ask since it
> >implies that the policy really is CTR for merge requests. (FWIW, I was
> >the lone -1 on the merge request, otherwise it likely would have gone in
> >given it already had 2 +1¹s despite a) breaking the build b) having
> >significant license issues and c) no design doc.)

Let me clarify that the policy is NOT CTR for merging branches back to
mainline.  It is RTC, with the added proviso that you need 3 +1 votes
from committers.  There is also a 5-day waiting period before you can
merge, so that people have time to chime in and possibly ask for changes
prior to the merge, or express a -1 vote.

I'm concerned that this discussion seems to be focusing on "how can I
use procedures and rules to avoid the need to get reviews from the
community" rather than "how can I get the community interested in giving
me reviews and feedback."

We still haven't spoken a word about what this feature or improvement
does, or why we would want it.  It is hard to say whether a feature
branch makes sense without that context.

best,
Colin
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Allen Wittenauer-2
In reply to this post by Chris Nauroth

> On Mar 23, 2016, at 10:25 AM, Chris Nauroth <[hidden email]> wrote:
>
> 2. Apache feature branches: Sign-off may come from designated branch
> committers in addition to full committers.  It's OK to break the branch
> for work in progress, but it must be fixed before a merge.  It's still
> review then commit though.

...

> Allen, are there particular reasons that you favor #2 (but with
> commit-then-review) instead of #3 for your work on HADOOP-12857 and
> HADOOP-12930?  Maybe you want the patches on Apache infrastructure so you
> can get the pre-commit sign-offs quickly?  Is it something else?

        HADOOP-12930 is sort of a weird case. I’ve only written some test code, but... ignoring unit tests and some variable name cleanup, there isn’t much code being truly added or modified.  However there is significant code _movement_ due to some refactoring that needs to take place.  As a result, this has some implications on reviewers, the testing infrastructure, and how it is committed:

* A combined patch file will be huge and overwhelming since a lot of effort will need to be spent actually finding the changes rather than having them as logically separated units of work.
* As all of you know, precommit doesn't work properly if any 2 or more of hadoop-common, hadoop-hdfs,  hadoop-mapreduce-project, and a handful of others are touched due to the time it takes to do the unit test runs. Guess how many of these are being touched? :)
* Committing piece meal means that feature-set wise, it could/would be incomplete until the last block is pushed into place.  This definitely needs to get committed as a logical set (either as many smaller commits at once or a rebased/merged single commit) in trunk.
* Being in a branch means that reviewers can checkout and test rather than having to apply a series of patches [despite how easy smart-apply-patch makes it. ;) ]
* One of the big criticisms during the 9902 review was that it would have been better to do it as a feature branch due to the size. This is me trying to acquiesce to that request. (It’s worthwhile pointing out that 9902 couldn’t have been easily broken up, however, since hadoop-config.sh was touched. That was a major ‘beak the world’ moment.)

        A big problem with the PMC’s ruleset around feature branches is that they are pretty much built around the idea that teams of people are going to be working on a feature. Realistically, I don’t see that being the case here (despite some mild interest from other committers in the feature) esp given I can probably finish this feature off in a few days.  That’s why I’m asking if CTR into the branch followed by RTM into trunk is a ‘legal' strategy here, since branch merges require reviews anyway.  

        Yes, I could make a branch in my own repo on gitlab, but I doubt that qualifies as something that is actually reviewable by the ASF’s rules…. which brings us back here anyway if I turn this into a giant patch file.
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Karthik Kambatla-2
A feature branch seems reasonable for this work: multiple reviewable
patches that are meaningful only after all of them get in. I guess most
comments here that had concerns for a branch were primarily based on the
initial reasoning of not being able to find a reviewer.

Coming to RTC and CTR, I haven't seen any branch that used CTR. May be, we
should consider allowing CTR. But, if it is hard to get reviews done for
individual smaller patches, I would think it would be hard to find 3
committers to review them later JIRA-wise or the combined patch.

On Wed, Mar 23, 2016 at 12:32 PM, Allen Wittenauer <[hidden email]> wrote:

>
> > On Mar 23, 2016, at 10:25 AM, Chris Nauroth <[hidden email]>
> wrote:
> >
> > 2. Apache feature branches: Sign-off may come from designated branch
> > committers in addition to full committers.  It's OK to break the branch
> > for work in progress, but it must be fixed before a merge.  It's still
> > review then commit though.
>
> ...
>
> > Allen, are there particular reasons that you favor #2 (but with
> > commit-then-review) instead of #3 for your work on HADOOP-12857 and
> > HADOOP-12930?  Maybe you want the patches on Apache infrastructure so you
> > can get the pre-commit sign-offs quickly?  Is it something else?
>
>         HADOOP-12930 is sort of a weird case. I’ve only written some test
> code, but... ignoring unit tests and some variable name cleanup, there
> isn’t much code being truly added or modified.  However there is
> significant code _movement_ due to some refactoring that needs to take
> place.  As a result, this has some implications on reviewers, the testing
> infrastructure, and how it is committed:
>
> * A combined patch file will be huge and overwhelming since a lot of
> effort will need to be spent actually finding the changes rather than
> having them as logically separated units of work.
> * As all of you know, precommit doesn't work properly if any 2 or more of
> hadoop-common, hadoop-hdfs,  hadoop-mapreduce-project, and a handful of
> others are touched due to the time it takes to do the unit test runs. Guess
> how many of these are being touched? :)
> * Committing piece meal means that feature-set wise, it could/would be
> incomplete until the last block is pushed into place.  This definitely
> needs to get committed as a logical set (either as many smaller commits at
> once or a rebased/merged single commit) in trunk.
> * Being in a branch means that reviewers can checkout and test rather than
> having to apply a series of patches [despite how easy smart-apply-patch
> makes it. ;) ]
> * One of the big criticisms during the 9902 review was that it would have
> been better to do it as a feature branch due to the size. This is me trying
> to acquiesce to that request. (It’s worthwhile pointing out that 9902
> couldn’t have been easily broken up, however, since hadoop-config.sh was
> touched. That was a major ‘beak the world’ moment.)
>
>         A big problem with the PMC’s ruleset around feature branches is
> that they are pretty much built around the idea that teams of people are
> going to be working on a feature. Realistically, I don’t see that being the
> case here (despite some mild interest from other committers in the feature)
> esp given I can probably finish this feature off in a few days.  That’s why
> I’m asking if CTR into the branch followed by RTM into trunk is a ‘legal'
> strategy here, since branch merges require reviews anyway.
>
>         Yes, I could make a branch in my own repo on gitlab, but I doubt
> that qualifies as something that is actually reviewable by the ASF’s
> rules…. which brings us back here anyway if I turn this into a giant patch
> file.
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Steve Loughran-3
In reply to this post by Allen Wittenauer-2

> On 22 Mar 2016, at 16:14, Allen Wittenauer <[hidden email]> wrote:
>
>
> Since it’s nearly impossible for me to get timely reviews for some build and script changes, is it possible for me to setup a branch, self review+commit to that branch, then request a branch merge?  I’m basically looking at doing this for HADOOP-12857 + HADOOP-12930 and their subtasks esp since they are very order dependent.  I don’t know if a branch is easier to review, honestly, but at least I wouldn’t be blocked on making progress...
>
> Thanks.

+1

I've seen your patches but they are beyond the ability of me to handle. If you can iterate around then you've got one branch for evolving & testing and then one merge when you want to get the final bits in

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Steve Loughran-3
In reply to this post by Andrew Wang

> On 22 Mar 2016, at 18:23, Andrew Wang <[hidden email]> wrote:
>
> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
> it's hard to find one reviewer, seems even harder to find two.

Given that only one +1 is needed to merge a non-branch patch, he could in theory convert the entire branch into a single .patch for review. Not that I'd encourage that, just observing that its possible


>
> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
> [hidden email]> wrote:
>
>>
>>> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]> wrote:
>>>
>>> That sounds like a reasonable approach and valid use of branches to me.
>>>
>>> Perhaps a set of functional tests could be provided/identified that would
>>> help the review process by showing backward compatibility along with new
>>> extensions for things like dynamic commands?
>>>
>>
>>        This is going into trunk, so no need for backward compatibility.
>>

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Chris Nauroth
In reply to this post by Karthik Kambatla-2
I think this is good technical justification for commit-then-review in a
feature branch, and I would be +1 to support it.

Just to briefly summarize one of my earlier comments, I see this as a way
to unblock solo development work and keep it moving in parallel while
rallying community members for reviews and any further communication.  I
don't see it as skirting process, because we still have the requirement
for binding +1s to get it into a shipping branch.  Since a feature branch
merge requires 3 +1s, it actually has the side effect of increasing the
burden for getting the code into a shipping branch.  There is a balance to
strike here.  I could see some medium-sized efforts going either way, and
ultimately it's up to the best judgment of the contributors and committers
on whether or not a feature branch is the best way to work.

BTW, for those not watching the JIRAs, HADOOP-12857 just got a committer
review, so I believe this is now just about HADOOP-12930.

--Chris Nauroth




On 3/23/16, 2:01 PM, "Karthik Kambatla" <[hidden email]> wrote:

>A feature branch seems reasonable for this work: multiple reviewable
>patches that are meaningful only after all of them get in. I guess most
>comments here that had concerns for a branch were primarily based on the
>initial reasoning of not being able to find a reviewer.
>
>Coming to RTC and CTR, I haven't seen any branch that used CTR. May be, we
>should consider allowing CTR. But, if it is hard to get reviews done for
>individual smaller patches, I would think it would be hard to find 3
>committers to review them later JIRA-wise or the combined patch.
>
>On Wed, Mar 23, 2016 at 12:32 PM, Allen Wittenauer <[hidden email]> wrote:
>
>>
>> > On Mar 23, 2016, at 10:25 AM, Chris Nauroth <[hidden email]>
>> wrote:
>> >
>> > 2. Apache feature branches: Sign-off may come from designated branch
>> > committers in addition to full committers.  It's OK to break the
>>branch
>> > for work in progress, but it must be fixed before a merge.  It's still
>> > review then commit though.
>>
>> ...
>>
>> > Allen, are there particular reasons that you favor #2 (but with
>> > commit-then-review) instead of #3 for your work on HADOOP-12857 and
>> > HADOOP-12930?  Maybe you want the patches on Apache infrastructure so
>>you
>> > can get the pre-commit sign-offs quickly?  Is it something else?
>>
>>         HADOOP-12930 is sort of a weird case. I¹ve only written some
>>test
>> code, but... ignoring unit tests and some variable name cleanup, there
>> isn¹t much code being truly added or modified.  However there is
>> significant code _movement_ due to some refactoring that needs to take
>> place.  As a result, this has some implications on reviewers, the
>>testing
>> infrastructure, and how it is committed:
>>
>> * A combined patch file will be huge and overwhelming since a lot of
>> effort will need to be spent actually finding the changes rather than
>> having them as logically separated units of work.
>> * As all of you know, precommit doesn't work properly if any 2 or more
>>of
>> hadoop-common, hadoop-hdfs,  hadoop-mapreduce-project, and a handful of
>> others are touched due to the time it takes to do the unit test runs.
>>Guess
>> how many of these are being touched? :)
>> * Committing piece meal means that feature-set wise, it could/would be
>> incomplete until the last block is pushed into place.  This definitely
>> needs to get committed as a logical set (either as many smaller commits
>>at
>> once or a rebased/merged single commit) in trunk.
>> * Being in a branch means that reviewers can checkout and test rather
>>than
>> having to apply a series of patches [despite how easy smart-apply-patch
>> makes it. ;) ]
>> * One of the big criticisms during the 9902 review was that it would
>>have
>> been better to do it as a feature branch due to the size. This is me
>>trying
>> to acquiesce to that request. (It¹s worthwhile pointing out that 9902
>> couldn¹t have been easily broken up, however, since hadoop-config.sh was
>> touched. That was a major Œbeak the world¹ moment.)
>>
>>         A big problem with the PMC¹s ruleset around feature branches is
>> that they are pretty much built around the idea that teams of people are
>> going to be working on a feature. Realistically, I don¹t see that being
>>the
>> case here (despite some mild interest from other committers in the
>>feature)
>> esp given I can probably finish this feature off in a few days.  That¹s
>>why
>> I¹m asking if CTR into the branch followed by RTM into trunk is a
>>Œlegal'
>> strategy here, since branch merges require reviews anyway.
>>
>>         Yes, I could make a branch in my own repo on gitlab, but I doubt
>> that qualifies as something that is actually reviewable by the ASF¹s
>> rulesŠ. which brings us back here anyway if I turn this into a giant
>>patch
>> file.

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Chris Nauroth
In reply to this post by Steve Loughran-3
It's interesting to go back to the change in bylaws in 2011 that
introduced the requirement for 3 binding +1s on a branch merge [1].  The
text of that resolution suggests that it's supportive of
commit-then-review if that's what the developers on the branch want to do.

"Branches' commit requirements are determined by the branch maintainer and
in this situation are often set up as commit-then-review."

It would also be very much against the spirit of that resolution to
combine it all down into a single patch file and get a single +1.

"As such, there is no way to guarantee that the entire change set offered
for trunk merge has had a second pair of eyes on it.  Therefore, it is
prudent to give that final merge heightened scrutiny, particularly since
these branches often extensively affect critical parts of the system.
Requiring three binding +1s does not slow down the branch development
process, but does provide a better chance of catching bugs before they
make their way to trunk."

--Chris Nauroth

[1] https://s.apache.org/iW1F



On 3/23/16, 2:11 PM, "Steve Loughran" <[hidden email]> wrote:

>
>> On 22 Mar 2016, at 18:23, Andrew Wang <[hidden email]> wrote:
>>
>> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
>> it's hard to find one reviewer, seems even harder to find two.
>
>Given that only one +1 is needed to merge a non-branch patch, he could in
>theory convert the entire branch into a single .patch for review. Not
>that I'd encourage that, just observing that its possible
>
>
>>
>> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
>> [hidden email]> wrote:
>>
>>>
>>>> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]>
>>>>wrote:
>>>>
>>>> That sounds like a reasonable approach and valid use of branches to
>>>>me.
>>>>
>>>> Perhaps a set of functional tests could be provided/identified that
>>>>would
>>>> help the review process by showing backward compatibility along with
>>>>new
>>>> extensions for things like dynamic commands?
>>>>
>>>
>>>        This is going into trunk, so no need for backward compatibility.
>>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

larry mccay-2
Thanks for digging that up, Chris.
That is completely what I would have expected but began questioning it
given this thread.

I think that Allen's use of a feature branch for this effort makes sense
and that he should have the freedom to choose his commit policy for the
branch.
The tricky part will be getting the reviews at the end but I would imagine
that it can be managed with some documentation, code review, tests and
instructions.

On Wed, Mar 23, 2016 at 5:20 PM, Chris Nauroth <[hidden email]>
wrote:

> It's interesting to go back to the change in bylaws in 2011 that
> introduced the requirement for 3 binding +1s on a branch merge [1].  The
> text of that resolution suggests that it's supportive of
> commit-then-review if that's what the developers on the branch want to do.
>
> "Branches' commit requirements are determined by the branch maintainer and
> in this situation are often set up as commit-then-review."
>
> It would also be very much against the spirit of that resolution to
> combine it all down into a single patch file and get a single +1.
>
> "As such, there is no way to guarantee that the entire change set offered
> for trunk merge has had a second pair of eyes on it.  Therefore, it is
> prudent to give that final merge heightened scrutiny, particularly since
> these branches often extensively affect critical parts of the system.
> Requiring three binding +1s does not slow down the branch development
> process, but does provide a better chance of catching bugs before they
> make their way to trunk."
>
> --Chris Nauroth
>
> [1] https://s.apache.org/iW1F
>
>
>
> On 3/23/16, 2:11 PM, "Steve Loughran" <[hidden email]> wrote:
>
> >
> >> On 22 Mar 2016, at 18:23, Andrew Wang <[hidden email]> wrote:
> >>
> >> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
> >> it's hard to find one reviewer, seems even harder to find two.
> >
> >Given that only one +1 is needed to merge a non-branch patch, he could in
> >theory convert the entire branch into a single .patch for review. Not
> >that I'd encourage that, just observing that its possible
> >
> >
> >>
> >> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
> >> [hidden email]> wrote:
> >>
> >>>
> >>>> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]>
> >>>>wrote:
> >>>>
> >>>> That sounds like a reasonable approach and valid use of branches to
> >>>>me.
> >>>>
> >>>> Perhaps a set of functional tests could be provided/identified that
> >>>>would
> >>>> help the review process by showing backward compatibility along with
> >>>>new
> >>>> extensions for things like dynamic commands?
> >>>>
> >>>
> >>>        This is going into trunk, so no need for backward compatibility.
> >>>
> >
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Chris Douglas
In reply to this post by Chris Nauroth
CTR is- and always has been- admissible in a branch.

On Wed, Mar 23, 2016 at 2:11 PM, Steve Loughran <[hidden email]> wrote:
> Given that only one +1 is needed to merge a non-branch patch, he could in theory convert the entire branch into a single .patch for review. Not that I'd encourage that, just observing that its possible

That might be correct. If the branch is exploring an idea and a single
patch is sufficient for review, then that's not exploiting a loophole
but developing in the open. If it's a cynical end-run around the
community, then the patch will be vetoed and the change reverted. -C

On Wed, Mar 23, 2016 at 2:20 PM, Chris Nauroth <[hidden email]> wrote:

> It's interesting to go back to the change in bylaws in 2011 that
> introduced the requirement for 3 binding +1s on a branch merge [1].  The
> text of that resolution suggests that it's supportive of
> commit-then-review if that's what the developers on the branch want to do.
>
> "Branches' commit requirements are determined by the branch maintainer and
> in this situation are often set up as commit-then-review."
>
> It would also be very much against the spirit of that resolution to
> combine it all down into a single patch file and get a single +1.
>
> "As such, there is no way to guarantee that the entire change set offered
> for trunk merge has had a second pair of eyes on it.  Therefore, it is
> prudent to give that final merge heightened scrutiny, particularly since
> these branches often extensively affect critical parts of the system.
> Requiring three binding +1s does not slow down the branch development
> process, but does provide a better chance of catching bugs before they
> make their way to trunk."
>
> --Chris Nauroth
>
> [1] https://s.apache.org/iW1F
>
>
>
> On 3/23/16, 2:11 PM, "Steve Loughran" <[hidden email]> wrote:
>
>>
>>> On 22 Mar 2016, at 18:23, Andrew Wang <[hidden email]> wrote:
>>>
>>> A branch sounds fine, but how are we going to get 3 +1's to merge it? If
>>> it's hard to find one reviewer, seems even harder to find two.
>>
>>Given that only one +1 is needed to merge a non-branch patch, he could in
>>theory convert the entire branch into a single .patch for review. Not
>>that I'd encourage that, just observing that its possible
>>
>>
>>>
>>> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
>>> [hidden email]> wrote:
>>>
>>>>
>>>>> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]>
>>>>>wrote:
>>>>>
>>>>> That sounds like a reasonable approach and valid use of branches to
>>>>>me.
>>>>>
>>>>> Perhaps a set of functional tests could be provided/identified that
>>>>>would
>>>>> help the review process by showing backward compatibility along with
>>>>>new
>>>>> extensions for things like dynamic commands?
>>>>>
>>>>
>>>>        This is going into trunk, so no need for backward compatibility.
>>>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Branch policy question

Gangumalla, Uma
In reply to this post by larry mccay-2
Thanks Chris N for digging back on this details.

The main concern on this question was started like ³Since it¹s nearly
impossible for me to get timely reviews for some build and script changes
Š.². So, Even for CTR process, review is needed thing at some point. I
have one question here is CTR tracks for each commit level reviews even if
they are later? Or we just commit, can review like bulk? I understand now
it may be branch managers decision. I feel this bulk review may be
problematic if it comes directly when merge vote comes. I am sure review
with that merge patch ( bigger patch containing all the branch changes)
may not be that efficient than smaller level patch reviews. It is ok, if
there is a way and reviewer reviews for each commit later, even after
commit with CTR process. But if commits get accumulated with out reviews
and to review whole changes at once at merge time, it may be much harder
to review as bulk. In this current discussion case also, much hard to get
some reviewers to review for a bulk patch. Is it make sense to leave JIRAs
open until they get reviewed even if you commit the patch with CTR in
branches (may be JIRA state can show something like ³In Review²)? So, that
patches will get reviewed before coming to merge votes.

Regards,
Uma  

On 3/23/16, 6:56 PM, "larry mccay" <[hidden email]> wrote:

>Thanks for digging that up, Chris.
>That is completely what I would have expected but began questioning it
>given this thread.
>
>I think that Allen's use of a feature branch for this effort makes sense
>and that he should have the freedom to choose his commit policy for the
>branch.
>The tricky part will be getting the reviews at the end but I would imagine
>that it can be managed with some documentation, code review, tests and
>instructions.
>
>On Wed, Mar 23, 2016 at 5:20 PM, Chris Nauroth <[hidden email]>
>wrote:
>
>> It's interesting to go back to the change in bylaws in 2011 that
>> introduced the requirement for 3 binding +1s on a branch merge [1].  The
>> text of that resolution suggests that it's supportive of
>> commit-then-review if that's what the developers on the branch want to
>>do.
>>
>> "Branches' commit requirements are determined by the branch maintainer
>>and
>> in this situation are often set up as commit-then-review."
>>
>> It would also be very much against the spirit of that resolution to
>> combine it all down into a single patch file and get a single +1.
>>
>> "As such, there is no way to guarantee that the entire change set
>>offered
>> for trunk merge has had a second pair of eyes on it.  Therefore, it is
>> prudent to give that final merge heightened scrutiny, particularly since
>> these branches often extensively affect critical parts of the system.
>> Requiring three binding +1s does not slow down the branch development
>> process, but does provide a better chance of catching bugs before they
>> make their way to trunk."
>>
>> --Chris Nauroth
>>
>> [1] https://s.apache.org/iW1F
>>
>>
>>
>> On 3/23/16, 2:11 PM, "Steve Loughran" <[hidden email]> wrote:
>>
>> >
>> >> On 22 Mar 2016, at 18:23, Andrew Wang <[hidden email]>
>>wrote:
>> >>
>> >> A branch sounds fine, but how are we going to get 3 +1's to merge
>>it? If
>> >> it's hard to find one reviewer, seems even harder to find two.
>> >
>> >Given that only one +1 is needed to merge a non-branch patch, he could
>>in
>> >theory convert the entire branch into a single .patch for review. Not
>> >that I'd encourage that, just observing that its possible
>> >
>> >
>> >>
>> >> On Tue, Mar 22, 2016 at 10:56 AM, Allen Wittenauer <
>> >> [hidden email]> wrote:
>> >>
>> >>>
>> >>>> On Mar 22, 2016, at 10:49 AM, larry mccay <[hidden email]>
>> >>>>wrote:
>> >>>>
>> >>>> That sounds like a reasonable approach and valid use of branches to
>> >>>>me.
>> >>>>
>> >>>> Perhaps a set of functional tests could be provided/identified that
>> >>>>would
>> >>>> help the review process by showing backward compatibility along
>>with
>> >>>>new
>> >>>> extensions for things like dynamic commands?
>> >>>>
>> >>>
>> >>>        This is going into trunk, so no need for backward
>>compatibility.
>> >>>
>> >
>> >
>>
>>

12