Re: [DISCUSS] Merging master -> feature branch

[Frances Perry]

Great, let's document that in the feature branch section of the
contribution guide:
http://beam.incubator.apache.org/contribute/contribution-guide/#feature-branches

Anyone want to take that?

On Thu, Oct 27, 2016 at 1:01 PM, Kenneth Knowles 
wrote:

> In the spirit of explicitly summarizing and concluding threads on list: I
> think we have affirmative consensus to go for it when a downstream
> integration is completely conflict-free and fixup-free.
>
> On Thu, Oct 27, 2016 at 12:43 PM Robert Bradshaw
>  wrote:
>
> > My concern was mostly about what to do in the face of conflicts, but
> > it sounds like the consensus is that for a clean merge, with no
> > conflicts or test breakage (or other concerns) a committer is free to
> > push without any oversight which is fine by me.
> >
> > [If/when the Mergbot comes into action, and runs more extensive tests
> > than standard precommit, it might make sense to still go through that
> > rather than debug bad merges discovered in postcommit tests.]
> >
> > On Wed, Oct 26, 2016 at 9:07 PM, Davor Bonaci 
> > wrote:
> > > +1
> > >
> > > I concur it is fine to proceed with a downstream integration (master ->
> > > feature branch -> sub-feature branch) without waiting for review for a
> > > completely clean merge. Exactly as proposed -- I think there should
> still
> > > be a pull request and comment saying it is a clean merge. (In some
> ideal
> > > world, this would happen nightly by a tool automatically, but I think
> > > that's not feasible in the short term.)
> > >
> > > I think other cases (upstream integration, merge conflict, any manual
> > > action, etc.) should still wait for a normal review.
> > >
> > > On Wed, Oct 26, 2016 at 10:34 AM, Thomas Weise  wrote:
> > >
> > >> +1
> > >>
> > >> For a merge from master to the feature branch that does not require
> > extra
> > >> changes, RTC does not add value. It actually delays and burns reviewer
> > time
> > >> (even mechanics need some) that "real" PRs could benefit from. If
> > >> adjustments are needed, then the regular process kicks in.
> > >>
> > >> Thanks,
> > >> Thomas
> > >>
> > >>
> > >> On Wed, Oct 26, 2016 at 1:33 AM, Amit Sela 
> > wrote:
> > >>
> > >> > I generally agree with Kenneth.
> > >> >
> > >> > While working on the SparkRunnerV2 branch, it was a pain - i avoided
> > >> > frequent merges to avoid trivial PRs, but it cost me with very large
> > and
> > >> > non-trivial merges later.
> > >> > I think that frequent merges for feature-branches should most of the
> > time
> > >> > be trivial (no conflicts) and a committer should be allowed to
> > self-merge
> > >> > once tests pass.
> > >> > As for conflicts, even for the smallest once I'd go with review just
> > so
> > >> > it's very clear when self-merging is OK - we can always revisit this
> > >> later
> > >> > and further discuss if we think we can improve this process.
> > >> >
> > >> > I guess +1 from me.
> > >> >
> > >> > Thanks,
> > >> > Amit.
> > >> >
> > >> > On Wed, Oct 26, 2016 at 8:10 AM Frances Perry
>  > >
> > >> > wrote:
> > >> >
> > >> > > On Tue, Oct 25, 2016 at 9:44 PM, Jean-Baptiste Onofré <
> > j...@nanthrax.net
> > >> >
> > >> > > wrote:
> > >> > >
> > >> > > > Agree. When possible it would be great to have the branch merged
> > on
> > >> > > master
> > >> > > > quickly, even when it's not fully ready. It would give more
> > >> visibility
> > >> > to
> > >> > > > potential contributors.
> > >> > > >
> > >> > >
> > >> > > This thread is about the opposite, I think -- merging master into
> > >> feature
> > >> > > branches regularly to prevent them from getting out of sync.
> > >> > >
> > >> > > As for increasing the visibility of feature branches, we have
> these
> > new
> > >> > > webpages:
> > >> > > http://beam.incubator.apache.org/contribute/work-in-progress/
> > >> > > http://beam.incubator.apache.org/contribute/contribution-
> > >> > > guide/#feature-branches
> > >> > > with more changes coming in the basic SDK/Runner landing pages
> too.
> > >> > >
> > >> >
> > >>
> >
>

Re: [DISCUSS] Merging master -> feature branch

[Kenneth Knowles]

In the spirit of explicitly summarizing and concluding threads on list: I
think we have affirmative consensus to go for it when a downstream
integration is completely conflict-free and fixup-free.

On Thu, Oct 27, 2016 at 12:43 PM Robert Bradshaw
 wrote:

> My concern was mostly about what to do in the face of conflicts, but
> it sounds like the consensus is that for a clean merge, with no
> conflicts or test breakage (or other concerns) a committer is free to
> push without any oversight which is fine by me.
>
> [If/when the Mergbot comes into action, and runs more extensive tests
> than standard precommit, it might make sense to still go through that
> rather than debug bad merges discovered in postcommit tests.]
>
> On Wed, Oct 26, 2016 at 9:07 PM, Davor Bonaci 
> wrote:
> > +1
> >
> > I concur it is fine to proceed with a downstream integration (master ->
> > feature branch -> sub-feature branch) without waiting for review for a
> > completely clean merge. Exactly as proposed -- I think there should still
> > be a pull request and comment saying it is a clean merge. (In some ideal
> > world, this would happen nightly by a tool automatically, but I think
> > that's not feasible in the short term.)
> >
> > I think other cases (upstream integration, merge conflict, any manual
> > action, etc.) should still wait for a normal review.
> >
> > On Wed, Oct 26, 2016 at 10:34 AM, Thomas Weise  wrote:
> >
> >> +1
> >>
> >> For a merge from master to the feature branch that does not require
> extra
> >> changes, RTC does not add value. It actually delays and burns reviewer
> time
> >> (even mechanics need some) that "real" PRs could benefit from. If
> >> adjustments are needed, then the regular process kicks in.
> >>
> >> Thanks,
> >> Thomas
> >>
> >>
> >> On Wed, Oct 26, 2016 at 1:33 AM, Amit Sela 
> wrote:
> >>
> >> > I generally agree with Kenneth.
> >> >
> >> > While working on the SparkRunnerV2 branch, it was a pain - i avoided
> >> > frequent merges to avoid trivial PRs, but it cost me with very large
> and
> >> > non-trivial merges later.
> >> > I think that frequent merges for feature-branches should most of the
> time
> >> > be trivial (no conflicts) and a committer should be allowed to
> self-merge
> >> > once tests pass.
> >> > As for conflicts, even for the smallest once I'd go with review just
> so
> >> > it's very clear when self-merging is OK - we can always revisit this
> >> later
> >> > and further discuss if we think we can improve this process.
> >> >
> >> > I guess +1 from me.
> >> >
> >> > Thanks,
> >> > Amit.
> >> >
> >> > On Wed, Oct 26, 2016 at 8:10 AM Frances Perry  >
> >> > wrote:
> >> >
> >> > > On Tue, Oct 25, 2016 at 9:44 PM, Jean-Baptiste Onofré <
> j...@nanthrax.net
> >> >
> >> > > wrote:
> >> > >
> >> > > > Agree. When possible it would be great to have the branch merged
> on
> >> > > master
> >> > > > quickly, even when it's not fully ready. It would give more
> >> visibility
> >> > to
> >> > > > potential contributors.
> >> > > >
> >> > >
> >> > > This thread is about the opposite, I think -- merging master into
> >> feature
> >> > > branches regularly to prevent them from getting out of sync.
> >> > >
> >> > > As for increasing the visibility of feature branches, we have these
> new
> >> > > webpages:
> >> > > http://beam.incubator.apache.org/contribute/work-in-progress/
> >> > > http://beam.incubator.apache.org/contribute/contribution-
> >> > > guide/#feature-branches
> >> > > with more changes coming in the basic SDK/Runner landing pages too.
> >> > >
> >> >
> >>
>

Re: [DISCUSS] Merging master -> feature branch

[Robert Bradshaw]

My concern was mostly about what to do in the face of conflicts, but
it sounds like the consensus is that for a clean merge, with no
conflicts or test breakage (or other concerns) a committer is free to
push without any oversight which is fine by me.

[If/when the Mergbot comes into action, and runs more extensive tests
than standard precommit, it might make sense to still go through that
rather than debug bad merges discovered in postcommit tests.]

On Wed, Oct 26, 2016 at 9:07 PM, Davor Bonaci  wrote:
> +1
>
> I concur it is fine to proceed with a downstream integration (master ->
> feature branch -> sub-feature branch) without waiting for review for a
> completely clean merge. Exactly as proposed -- I think there should still
> be a pull request and comment saying it is a clean merge. (In some ideal
> world, this would happen nightly by a tool automatically, but I think
> that's not feasible in the short term.)
>
> I think other cases (upstream integration, merge conflict, any manual
> action, etc.) should still wait for a normal review.
>
> On Wed, Oct 26, 2016 at 10:34 AM, Thomas Weise  wrote:
>
>> +1
>>
>> For a merge from master to the feature branch that does not require extra
>> changes, RTC does not add value. It actually delays and burns reviewer time
>> (even mechanics need some) that "real" PRs could benefit from. If
>> adjustments are needed, then the regular process kicks in.
>>
>> Thanks,
>> Thomas
>>
>>
>> On Wed, Oct 26, 2016 at 1:33 AM, Amit Sela  wrote:
>>
>> > I generally agree with Kenneth.
>> >
>> > While working on the SparkRunnerV2 branch, it was a pain - i avoided
>> > frequent merges to avoid trivial PRs, but it cost me with very large and
>> > non-trivial merges later.
>> > I think that frequent merges for feature-branches should most of the time
>> > be trivial (no conflicts) and a committer should be allowed to self-merge
>> > once tests pass.
>> > As for conflicts, even for the smallest once I'd go with review just so
>> > it's very clear when self-merging is OK - we can always revisit this
>> later
>> > and further discuss if we think we can improve this process.
>> >
>> > I guess +1 from me.
>> >
>> > Thanks,
>> > Amit.
>> >
>> > On Wed, Oct 26, 2016 at 8:10 AM Frances Perry 
>> > wrote:
>> >
>> > > On Tue, Oct 25, 2016 at 9:44 PM, Jean-Baptiste Onofré > >
>> > > wrote:
>> > >
>> > > > Agree. When possible it would be great to have the branch merged on
>> > > master
>> > > > quickly, even when it's not fully ready. It would give more
>> visibility
>> > to
>> > > > potential contributors.
>> > > >
>> > >
>> > > This thread is about the opposite, I think -- merging master into
>> feature
>> > > branches regularly to prevent them from getting out of sync.
>> > >
>> > > As for increasing the visibility of feature branches, we have these new
>> > > webpages:
>> > > http://beam.incubator.apache.org/contribute/work-in-progress/
>> > > http://beam.incubator.apache.org/contribute/contribution-
>> > > guide/#feature-branches
>> > > with more changes coming in the basic SDK/Runner landing pages too.
>> > >
>> >
>>

Re: [DISCUSS] Merging master -> feature branch

[Thomas Weise]

+1

For a merge from master to the feature branch that does not require extra
changes, RTC does not add value. It actually delays and burns reviewer time
(even mechanics need some) that "real" PRs could benefit from. If
adjustments are needed, then the regular process kicks in.

Thanks,
Thomas


On Wed, Oct 26, 2016 at 1:33 AM, Amit Sela  wrote:

> I generally agree with Kenneth.
>
> While working on the SparkRunnerV2 branch, it was a pain - i avoided
> frequent merges to avoid trivial PRs, but it cost me with very large and
> non-trivial merges later.
> I think that frequent merges for feature-branches should most of the time
> be trivial (no conflicts) and a committer should be allowed to self-merge
> once tests pass.
> As for conflicts, even for the smallest once I'd go with review just so
> it's very clear when self-merging is OK - we can always revisit this later
> and further discuss if we think we can improve this process.
>
> I guess +1 from me.
>
> Thanks,
> Amit.
>
> On Wed, Oct 26, 2016 at 8:10 AM Frances Perry 
> wrote:
>
> > On Tue, Oct 25, 2016 at 9:44 PM, Jean-Baptiste Onofré 
> > wrote:
> >
> > > Agree. When possible it would be great to have the branch merged on
> > master
> > > quickly, even when it's not fully ready. It would give more visibility
> to
> > > potential contributors.
> > >
> >
> > This thread is about the opposite, I think -- merging master into feature
> > branches regularly to prevent them from getting out of sync.
> >
> > As for increasing the visibility of feature branches, we have these new
> > webpages:
> > http://beam.incubator.apache.org/contribute/work-in-progress/
> > http://beam.incubator.apache.org/contribute/contribution-
> > guide/#feature-branches
> > with more changes coming in the basic SDK/Runner landing pages too.
> >
>

Re: [DISCUSS] Merging master -> feature branch

[Amit Sela]

I generally agree with Kenneth.

While working on the SparkRunnerV2 branch, it was a pain - i avoided
frequent merges to avoid trivial PRs, but it cost me with very large and
non-trivial merges later.
I think that frequent merges for feature-branches should most of the time
be trivial (no conflicts) and a committer should be allowed to self-merge
once tests pass.
As for conflicts, even for the smallest once I'd go with review just so
it's very clear when self-merging is OK - we can always revisit this later
and further discuss if we think we can improve this process.

I guess +1 from me.

Thanks,
Amit.

On Wed, Oct 26, 2016 at 8:10 AM Frances Perry 
wrote:

> On Tue, Oct 25, 2016 at 9:44 PM, Jean-Baptiste Onofré 
> wrote:
>
> > Agree. When possible it would be great to have the branch merged on
> master
> > quickly, even when it's not fully ready. It would give more visibility to
> > potential contributors.
> >
>
> This thread is about the opposite, I think -- merging master into feature
> branches regularly to prevent them from getting out of sync.
>
> As for increasing the visibility of feature branches, we have these new
> webpages:
> http://beam.incubator.apache.org/contribute/work-in-progress/
> http://beam.incubator.apache.org/contribute/contribution-
> guide/#feature-branches
> with more changes coming in the basic SDK/Runner landing pages too.
>

Re: [DISCUSS] Merging master -> feature branch

[Jean-Baptiste Onofré]

Agree. When possible it would be great to have the branch merged on master 
quickly, even when it's not fully ready. It would give more visibility to 
potential contributors.

Regards
JB

⁣​

On Oct 25, 2016, 23:34, at 23:34, Kenneth Knowles  
wrote:
>Hi all,
>
>While collaborating on the apex-runner branch, the issue of how best to
>continuously merge master into the feature branch came up. IMO it
>differs
>somewhat from normal commits in two notable ways:
>
>1. Modulo fix-ups, it is actually not adding any new code to the
>overall
>codebase, so reviews don't serve to raise the quality bar for
>contributions.
>2. It is pretty important to do very frequently to keep out of trouble,
>so
>friction must be thoroughly justified.
>
>I really wouldn't want reviewing a daily merge from master to consume
>someone's time every day. On the gearpump-runner branch we had some
>major
>review latency problems. Obviously, I'd like to hear from folks on
>other
>feature branches. How has this process been for the Python SDK?
>
>I will also throw out an idea for a variation in process:
>
>1. A committer merges master to their feature branch without
>conflicts.*
>2. They open a PR for the merge.
>3a. If the tests pass without modifications _the committer self-merges
>the
>PR without waiting for review_.
>3b. If there are any adjustments needed, these go in separate commits
>on
>the same PR and the review process is as usual (the reviewer can review
>just the added commits).
>
>What do you think? Does this treat such merges too blithely? This is
>meant
>just as a starting point for discussion.
>
>Kenn
>
>* There should never be real conflicts unless something strange has
>happened - the feature can't be edited on the master branch, and master
>stuff shouldn't be touched on the feature branch.

Re: [DISCUSS] Merging master -> feature branch

[Manu Zhang]

I like this idea as a maintainer for gearpump-runner branch. Usually the
merge is to keep the feature branch updated to the latest API changes on
master.

Sorry that I haven't made merge PR as frequent as possible which would
bring in a lot of changes each time and make it harder for others to
review. On the other hand, it's not easy for the feature branch to follow
master's steps since master has far more committers/contributors to work
on.

I like this idea but it won't change much for gearpump-runner branch since
we still need committers to merge.

Thanks,
Manu





On Wed, Oct 26, 2016 at 7:58 AM Robert Bradshaw 
wrote:

On Tue, Oct 25, 2016 at 2:33 PM, Kenneth Knowles 
wrote:
> Hi all,
>
> While collaborating on the apex-runner branch, the issue of how best to
> continuously merge master into the feature branch came up. IMO it differs
> somewhat from normal commits in two notable ways:
>
> 1. Modulo fix-ups, it is actually not adding any new code to the overall
> codebase, so reviews don't serve to raise the quality bar for
contributions.
> 2. It is pretty important to do very frequently to keep out of trouble, so
> friction must be thoroughly justified.
>
> I really wouldn't want reviewing a daily merge from master to consume
> someone's time every day. On the gearpump-runner branch we had some major
> review latency problems. Obviously, I'd like to hear from folks on other
> feature branches. How has this process been for the Python SDK?

The Python SDK sits at the extreme end of there being no conflicts,
and due to the paucity of intersection, little motivation to bother
merging at all.

> I will also throw out an idea for a variation in process:
>
> 1. A committer merges master to their feature branch without conflicts.*
> 2. They open a PR for the merge.
> 3a. If the tests pass without modifications _the committer self-merges the
> PR without waiting for review_.
> 3b. If there are any adjustments needed, these go in separate commits on
> the same PR and the review process is as usual (the reviewer can review
> just the added commits).
>
> What do you think? Does this treat such merges too blithely? This is meant
> just as a starting point for discussion.
>
> Kenn
>
> * There should never be real conflicts unless something strange has
> happened - the feature can't be edited on the master branch, and master
> stuff shouldn't be touched on the feature branch.

I think you're being a little optimistic about there rarely being a
need for conflict resolution--often work on a feature requires
refactoring/extending the main codebase. Hopefully as we provide a
clean (and stable) API between runners and SDKs this will still be the
case, but I don't think we're there yet.

If there are conflicts, does one check in the conflict markers then
resolve in a separate commit? Only for messy ones, or all the time?
Certainly if further adjustments are needed we should do a full
review.

In my opinion, reviewing a merge should not be too laborious a process
(e.g. one needn't necessarily read the entire diff as one would with a
standard commit). It shouldn't be blocking anyone to wait for a review
(one can always develop on top of the merge). If there's not enough
activity on a branch to do this, the branch has other troubles. So I'd
lean towards not making an exception here, but could be convinced
otherwise.

- Robert

[DISCUSS] Merging master -> feature branch

[Kenneth Knowles]

Hi all,

While collaborating on the apex-runner branch, the issue of how best to
continuously merge master into the feature branch came up. IMO it differs
somewhat from normal commits in two notable ways:

1. Modulo fix-ups, it is actually not adding any new code to the overall
codebase, so reviews don't serve to raise the quality bar for contributions.
2. It is pretty important to do very frequently to keep out of trouble, so
friction must be thoroughly justified.

I really wouldn't want reviewing a daily merge from master to consume
someone's time every day. On the gearpump-runner branch we had some major
review latency problems. Obviously, I'd like to hear from folks on other
feature branches. How has this process been for the Python SDK?

I will also throw out an idea for a variation in process:

1. A committer merges master to their feature branch without conflicts.*
2. They open a PR for the merge.
3a. If the tests pass without modifications _the committer self-merges the
PR without waiting for review_.
3b. If there are any adjustments needed, these go in separate commits on
the same PR and the review process is as usual (the reviewer can review
just the added commits).

What do you think? Does this treat such merges too blithely? This is meant
just as a starting point for discussion.

Kenn

* There should never be real conflicts unless something strange has
happened - the feature can't be edited on the master branch, and master
stuff shouldn't be touched on the feature branch.