subject:"PR merge policy"


On 4/29/17 15:51, Justin Mclean wrote:

HI,


Does anybody know if:
1. Any other Apache project has a similar forced-revert policy; and

None that I’m aware of. But if a committer checks in some code the breaks 
something and someone vetoes it, it is expected that they revert the checkin 
and fix it. I’ve seen this happen a few times, but mostly there a discussion 
and the issues fixed by additional checkins. This is usually quite rate.


2. Any Apache project has actually force-reverted a commit for any reason
in recent years, regardless of whether they have a policy about it or not.

Yes I’ve seen it use to fix up merge issue with long lived branches. But again 
very rare.
Master is the long lived branch and I do not expect that we will need to 
use force push frequently especially after this discussion.


Thanks,
Justin

Re: PR merge policy

2017-04-29 Thread Justin Mclean

HI,

> Does anybody know if:
> 1. Any other Apache project has a similar forced-revert policy; and

None that I’m aware of. But if a committer checks in some code the breaks 
something and someone vetoes it, it is expected that they revert the checkin 
and fix it. I’ve seen this happen a few times, but mostly there a discussion 
and the issues fixed by additional checkins. This is usually quite rate.

> 2. Any Apache project has actually force-reverted a commit for any reason
> in recent years, regardless of whether they have a policy about it or not.

Yes I’ve seen it use to fix up merge issue with long lived branches. But again 
very rare.

Thanks,
Justin

Re: PR merge policy

I will add one more question:

How many contributors/committers can tell how many force pushes were
done to apex repositories since graduation without looking into
commits@apex.

Thank you,

Vlad

On 4/29/17 11:48, Munagala Ramanath wrote:

Looks like we have clarity now that we are at an impasse: -1 on forced
revert and -1 on the
only other alternative with is additional commits. We need a continuing
resolution
(
https://federalnewsradio.com/federal-headlines/2017/04/short-term-spending-measure-introduced-to-keep-government-open/
)
to keep going :-)

Does anybody know if:
1. Any other Apache project has a similar forced-revert policy; and
2. Any Apache project has actually force-reverted a commit for any reason
in recent years, regardless
of whether they have a policy about it or not.

These additional data points will help in arriving at a resolution.

Ram

On Sat, Apr 29, 2017 at 10:46 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:

That is not the main point I was making. I think your main concern is that
when a commit gets added like this, the principle of putting community
first is being violated. That is well taken but what I am trying to say is
that your remedy is going the same route because you are focusing only on a
specific way of undoing the change and not considering how it is going to
affect the community. There is no silver bullet here but I think a few
extra commits are acceptable.

On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I don't think that anyone proposed shaming of individuals who violated

the

policy especially in a public e-mail. Understanding of inconvenience

caused

to the community is sufficient to avoid policy violations.

I would propose using PR to communicate to a committer and a contributor
request to undo the commit unless a commit was done without PR even being
open that will be a larger violation. In the later case, e-mail to

dev@apex

is required.

I am strongly against commit on top of a commit. Undo needs to be done
quickly and reverting changes in extra commit still require review to be
sure that "undo" is complete. In addition, after PR review is done, we

will

end up with 3 commits instead of one.

Thank you,

Vlad

On 4/29/17 08:29, Pramod Immaneni wrote:

I assume we are still referring to force push and removing the commit

from

the upstream git commit tree as rollback and not to applying a new

commit

that reverts the changes. When a violation happens, why should everyone
(who synced with the repo) suffer the inconvenience of their local git
repo/branch ending up with a bad git commit state for a problem they did
not cause, which would happen when rollback is employed, when this can

easily fixed by a new commit that reverts the changes. Second, a

violation

while bad, should the default policy be, to immediately revert the

changes

with a new PR/commit without looking at what they are? That seems
unnecessarily draconian too, considering that violations are not the

norm

nor flagrant that drastic measures need to be taken now. If the changes
are
questionable or even simply require more time for review, yes, by all
means, send an email to dev list with the reason for reverting the PR,
revert the changes with a new commit and redo the PR process. The email
serves two purposes, first it serves as a courtesy notification both to
the
committer and contributor as to why the commit is being reverted and
second it also ends up being a reminder to the committer that their

action

affected the broader community, that they need to be cognizant of it and
be
more careful in the future. Also, I am against any public shaming of
individuals in the emails.

Thanks

On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I think it is a good idea to make the committer responsible for fixing

the

situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect

the

community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad

On 4/28/17 14:21, Amol Kekre wrote:

Strongly agree with Ilya. Lets take these events as learning

opportunities
for folks to learn and improve. There can always be second commit to

fix

in
case there is code issue. If it is a policy issue, we learn and

improve.

Rolling back, should be used rarely and it does need to be a disaster.
We
need to be cognizant of new contributors worrying about the cost to
submit
code.

I too do not think Apex is hurting from bad code getting in. We are
doing
great with our current policies.

Thks,
Amol

E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com

On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
ilya.gane...@capitalone.com>
wrote:

Guess

Re: PR merge policy

I agree that there is no silver bullet here. My view on the force push 
is that it is not so disruptive to the community as it looks to be. It 
is very easy to reset master to apache/github remote master and rebase 
branches if necessary assuming that it is not done frequently. I view 
multiple commits as a bad practice and IMO they will cause more 
confusion in a longer run. In a few years it will be hard to understand 
why one commit was followed by an undo commit, followed by another 
commit without doing a lot of research (especially that those commits 
may not necessarily be sequential).


Thank you,

Vlad

On 4/29/17 10:46, Pramod Immaneni wrote:

That is not the main point I was making. I think your main concern is that
when a commit gets added like this, the principle of putting community
first is being violated. That is well taken but what I am trying to say is
that your remedy is going the same route because you are focusing only on a
specific way of undoing the change and not considering how it is going to
affect the community. There is no silver bullet here but I think a few
extra commits are acceptable.

On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


I don't think that anyone proposed shaming of individuals who violated the
policy especially in a public e-mail. Understanding of inconvenience caused
to the community is sufficient to avoid policy violations.

I would propose using PR to communicate to a committer and a contributor
request to undo the commit unless a commit was done without PR even being
open that will be a larger violation. In the later case, e-mail to dev@apex
is required.

I am strongly against commit on top of a commit. Undo needs to be done
quickly and reverting changes in extra commit still require review to be
sure that "undo" is complete. In addition, after PR review is done, we will
end up with 3 commits instead of one.

Thank you,

Vlad


On 4/29/17 08:29, Pramod Immaneni wrote:


I assume we are still referring to force push and removing the commit from
the upstream git commit tree as rollback and not to applying a new commit
that reverts the changes. When a violation happens, why should everyone
(who synced with the repo) suffer the inconvenience of their local git
repo/branch ending up with a bad git commit state for a problem they did
not cause, which would happen when rollback is employed, when this can be
easily fixed by a new commit that reverts the changes. Second, a violation
while bad, should the default policy be, to immediately revert the changes
with a new PR/commit without looking at what they are? That seems
unnecessarily draconian too, considering that violations are not the norm
nor flagrant that drastic measures need to be taken now. If the changes
are
questionable or even simply require more time for review, yes, by all
means, send an email to dev list with the reason for reverting the PR,
revert the changes with a new commit and redo the PR process. The email
serves two purposes, first it serves as a courtesy notification both to
the
committer and contributor as to why the commit is being reverted and
second it also ends up being a reminder to the committer that their action
affected the broader community, that they need to be cognizant of it and
be
more careful in the future. Also, I am against any public shaming of
individuals in the emails.

Thanks

On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I think it is a good idea to make the committer responsible for fixing the

situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect the
community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad


On 4/28/17 14:21, Amol Kekre wrote:

Strongly agree with Ilya. Lets take these events as learning

opportunities
for folks to learn and improve. There can always be second commit to fix
in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster.
We
need to be cognizant of new contributors worrying about the cost to
submit
code.

I too do not think Apex is hurting from bad code getting in. We are
doing
great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
ilya.gane...@capitalone.com>
wrote:

Guess we can all go home then. Our work here is done:




W.R.T the discussion below, I think rolling back an improperly reviewed
PR
could be considered disrespectful to the committer who merged it in the
first place. I think that such situations, unless they trigger a
disaster,
should be handled by communicating the error to the responsible

Re: PR merge policy

2017-04-29 Thread Munagala Ramanath

Looks like we have clarity now that we are at an impasse: -1 on forced
revert and -1 on the
only other alternative with is additional commits. We need a continuing
resolution
(
https://federalnewsradio.com/federal-headlines/2017/04/short-term-spending-measure-introduced-to-keep-government-open/
)
to keep going :-)

Does anybody know if:
1. Any other Apache project has a similar forced-revert policy; and
2. Any Apache project has actually force-reverted a commit for any reason
in recent years, regardless
of whether they have a policy about it or not.

These additional data points will help in arriving at a resolution.

Ram

On Sat, Apr 29, 2017 at 10:46 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:

> That is not the main point I was making. I think your main concern is that
> when a commit gets added like this, the principle of putting community
> first is being violated. That is well taken but what I am trying to say is
> that your remedy is going the same route because you are focusing only on a
> specific way of undoing the change and not considering how it is going to
> affect the community. There is no silver bullet here but I think a few
> extra commits are acceptable.
>
> On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> > I don't think that anyone proposed shaming of individuals who violated
> the
> > policy especially in a public e-mail. Understanding of inconvenience
> caused
> > to the community is sufficient to avoid policy violations.
> >
> > I would propose using PR to communicate to a committer and a contributor
> > request to undo the commit unless a commit was done without PR even being
> > open that will be a larger violation. In the later case, e-mail to
> dev@apex
> > is required.
> >
> > I am strongly against commit on top of a commit. Undo needs to be done
> > quickly and reverting changes in extra commit still require review to be
> > sure that "undo" is complete. In addition, after PR review is done, we
> will
> > end up with 3 commits instead of one.
> >
> > Thank you,
> >
> > Vlad
> >
> >
> > On 4/29/17 08:29, Pramod Immaneni wrote:
> >
> >> I assume we are still referring to force push and removing the commit
> from
> >> the upstream git commit tree as rollback and not to applying a new
> commit
> >> that reverts the changes. When a violation happens, why should everyone
> >> (who synced with the repo) suffer the inconvenience of their local git
> >> repo/branch ending up with a bad git commit state for a problem they did
> >> not cause, which would happen when rollback is employed, when this can
> be
> >> easily fixed by a new commit that reverts the changes. Second, a
> violation
> >> while bad, should the default policy be, to immediately revert the
> changes
> >> with a new PR/commit without looking at what they are? That seems
> >> unnecessarily draconian too, considering that violations are not the
> norm
> >> nor flagrant that drastic measures need to be taken now. If the changes
> >> are
> >> questionable or even simply require more time for review, yes, by all
> >> means, send an email to dev list with the reason for reverting the PR,
> >> revert the changes with a new commit and redo the PR process. The email
> >> serves two purposes, first it serves as a courtesy notification both to
> >> the
> >> committer and contributor as to why the commit is being reverted and
> >> second it also ends up being a reminder to the committer that their
> action
> >> affected the broader community, that they need to be cognizant of it and
> >> be
> >> more careful in the future. Also, I am against any public shaming of
> >> individuals in the emails.
> >>
> >> Thanks
> >>
> >> On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
> >> wrote:
> >>
> >> I think it is a good idea to make the committer responsible for fixing
> the
> >>> situation by rolling back the commit and re-opening the PR for further
> >>> review. IMO, committer right comes with the responsibility to respect
> the
> >>> community and policies it established.
> >>>
> >>> I would disagree that rolling back should be used only in case of a
> >>> disaster unless PR merge policy violation is a disaster :-) (and it
> >>> actually is).
> >>>
> >>> Thank you,
> >>>
> >>> Vlad
> >>>
> >>>
> >>> On 4/28/17 14:21, Amo

Re: PR merge policy

2017-04-29 Thread Pramod Immaneni

That is not the main point I was making. I think your main concern is that
when a commit gets added like this, the principle of putting community
first is being violated. That is well taken but what I am trying to say is
that your remedy is going the same route because you are focusing only on a
specific way of undoing the change and not considering how it is going to
affect the community. There is no silver bullet here but I think a few
extra commits are acceptable.

On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> I don't think that anyone proposed shaming of individuals who violated the
> policy especially in a public e-mail. Understanding of inconvenience caused
> to the community is sufficient to avoid policy violations.
>
> I would propose using PR to communicate to a committer and a contributor
> request to undo the commit unless a commit was done without PR even being
> open that will be a larger violation. In the later case, e-mail to dev@apex
> is required.
>
> I am strongly against commit on top of a commit. Undo needs to be done
> quickly and reverting changes in extra commit still require review to be
> sure that "undo" is complete. In addition, after PR review is done, we will
> end up with 3 commits instead of one.
>
> Thank you,
>
> Vlad
>
>
> On 4/29/17 08:29, Pramod Immaneni wrote:
>
>> I assume we are still referring to force push and removing the commit from
>> the upstream git commit tree as rollback and not to applying a new commit
>> that reverts the changes. When a violation happens, why should everyone
>> (who synced with the repo) suffer the inconvenience of their local git
>> repo/branch ending up with a bad git commit state for a problem they did
>> not cause, which would happen when rollback is employed, when this can be
>> easily fixed by a new commit that reverts the changes. Second, a violation
>> while bad, should the default policy be, to immediately revert the changes
>> with a new PR/commit without looking at what they are? That seems
>> unnecessarily draconian too, considering that violations are not the norm
>> nor flagrant that drastic measures need to be taken now. If the changes
>> are
>> questionable or even simply require more time for review, yes, by all
>> means, send an email to dev list with the reason for reverting the PR,
>> revert the changes with a new commit and redo the PR process. The email
>> serves two purposes, first it serves as a courtesy notification both to
>> the
>> committer and contributor as to why the commit is being reverted and
>> second it also ends up being a reminder to the committer that their action
>> affected the broader community, that they need to be cognizant of it and
>> be
>> more careful in the future. Also, I am against any public shaming of
>> individuals in the emails.
>>
>> Thanks
>>
>> On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
>> wrote:
>>
>> I think it is a good idea to make the committer responsible for fixing the
>>> situation by rolling back the commit and re-opening the PR for further
>>> review. IMO, committer right comes with the responsibility to respect the
>>> community and policies it established.
>>>
>>> I would disagree that rolling back should be used only in case of a
>>> disaster unless PR merge policy violation is a disaster :-) (and it
>>> actually is).
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>>
>>> On 4/28/17 14:21, Amol Kekre wrote:
>>>
>>> Strongly agree with Ilya. Lets take these events as learning
>>>> opportunities
>>>> for folks to learn and improve. There can always be second commit to fix
>>>> in
>>>> case there is code issue. If it is a policy issue, we learn and improve.
>>>> Rolling back, should be used rarely and it does need to be a disaster.
>>>> We
>>>> need to be cognizant of new contributors worrying about the cost to
>>>> submit
>>>> code.
>>>>
>>>> I too do not think Apex is hurting from bad code getting in. We are
>>>> doing
>>>> great with our current policies.
>>>>
>>>> Thks,
>>>> Amol
>>>>
>>>>
>>>> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>>>>
>>>> www.datatorrent.com
>>>>
>>>>
>>>> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
>>>> ilya.gane...@capitalone.com>
>>>> wrote:
>>&g

Re: PR merge policy

I am -1 on the proposal to submit new PR. If commit was done 
prematurely, it needs to be undone as quickly as it was done.


Thank you,

Vlad
On 4/29/17 09:34, Munagala Ramanath wrote:

Given the divergence of views on this topic, I think we need to clarify
what "last resort"
means; to me it means:

In most cases when PRs are prematurely merged, the preferred
mechanism to address it will be to open a new PR with corrections resulting
in a new
commit on top. A forced reversion of the prior commit will only occur only
when it is
judged to be disastrously flawed.

Ram



On Sat, Apr 29, 2017 at 8:33 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


Yes, the rollback is the last resort as Thomas mentioned. I hope that we
will not need it.

Possibly I misjudge, but I don't see forced push as an extremely
disturbing event for the community especially if it is done quickly.

Thank you,

Vlad

On 4/28/17 23:33, Bhupesh Chawda wrote:



Vlad,

Your point regarding not merging any PR without allowing the community to
review is well taken.
I agree that the committer should be responsible for rolling back the
change and
I think 
we should establish the way in which it should be done.

There
can
   be a delay for the committer to notice that a commit should not have
been
merged and needs to be reverted. But it might be a while before the
committer realizes this and a force push might create problems for the
community. Perhaps a new PR could be the way to go.

Note that I agree that
undoing a PR
   may not even be needed given that the community has had a chance to
review
it, but there might still be cases where such undo commits need to be
done.

~ Bhupesh


___

Bhupesh Chawda

E: bhup...@datatorrent.com | Twitter: @bhupeshsc

www.datatorrent.com  |  apex.apache.org



On Sat, Apr 29, 2017 at 11:07 AM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

If a committer is fast to pull the trigger to merge and slow to rollback

when the policy is violated, the case can be sent to PMC. It will be a
clear indication that a committer does not respect the community, so I
disagree that this is "just kicking the can down the road" as committer
right may be eventually revoked (hope we don't need to go that far ever).

Not all policies are immediately reflected at
http://apex.apache.org/contributing.html. The vote clearly happened a
week ago when I initially proposed that a PR can be merged only once the
community has a chance to review it . http://apex.apache.org/contrib
uting.html is for new contributors and new committers, existing
committers should be fully aware of the PR merge policy and if in doubt,
raise a question here (dev@apex).

We are all humans and may overlook problems in PRs that we review. The
concern is not that Travis build may fail and a commit needs to be
reverted. It will not be a valid reason to undo the commit (note that it
is
still committer responsibility to thoroughly review changes/new code and
ensure that license is in place, build is free from compilation and other
errors, code is optimal and readable, and basically put his/her name next
to the contributor name). The concern is when a committer does not give
community a chance for review. It is against "community before code"
behavior that we want to prohibit. I am strongly for requesting a
contributor to undo a commit in such cases and disallowing additional
"fix"
commits.

Thank you,

Vlad

On 4/28/17 20:17, Munagala Ramanath wrote:

That's just kicking the can down the road: What if the committer is not

inclined
to perform the rollback ? Other reviewers can provide feedback on the
closed PR
but the committer may choose to add a new commit on top.

Firstly, the point about waiting a day or two is not even a formal
policy
requirement
outlined at http://apex.apache.org/contributing.html in the "Merging a
Pull
Request"
section, so it only has an advisory status currently in my view.

Secondly, I think a variety of so called "violations" from overlooking
Travis build
failures, to not detecting missing unit tests, to not enforcing JavaDoc
requirements
are not fatal in most cases. Reverting commits in such cases is contrary
to
the
Apache injunction to "Put community before code" (
http://community.apache.org/newbiefaq.html)

So I am firmly against reverting except in the most dire circumstances
and
in favor
of adding new commits to fix issues -- we do that for bugs, no reason to
treat these
other cases differently.

Ram

On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <a...@datatorrent.com>
wrote:

That makes sense. The committer should fix upon feedback. PR policy


violation is bad, I am not defending the violation.  think if the
committer
takes the feedback and promptly works on a fix (new pr) it should be
ok.

Thks,
Amol



E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com

Re: PR merge policy

I don't think that anyone proposed shaming of individuals who violated 
the policy especially in a public e-mail. Understanding of inconvenience 
caused to the community is sufficient to avoid policy violations.


I would propose using PR to communicate to a committer and a contributor 
request to undo the commit unless a commit was done without PR even 
being open that will be a larger violation. In the later case, e-mail to 
dev@apex is required.


I am strongly against commit on top of a commit. Undo needs to be done 
quickly and reverting changes in extra commit still require review to be 
sure that "undo" is complete. In addition, after PR review is done, we 
will end up with 3 commits instead of one.


Thank you,

Vlad

On 4/29/17 08:29, Pramod Immaneni wrote:

I assume we are still referring to force push and removing the commit from
the upstream git commit tree as rollback and not to applying a new commit
that reverts the changes. When a violation happens, why should everyone
(who synced with the repo) suffer the inconvenience of their local git
repo/branch ending up with a bad git commit state for a problem they did
not cause, which would happen when rollback is employed, when this can be
easily fixed by a new commit that reverts the changes. Second, a violation
while bad, should the default policy be, to immediately revert the changes
with a new PR/commit without looking at what they are? That seems
unnecessarily draconian too, considering that violations are not the norm
nor flagrant that drastic measures need to be taken now. If the changes are
questionable or even simply require more time for review, yes, by all
means, send an email to dev list with the reason for reverting the PR,
revert the changes with a new commit and redo the PR process. The email
serves two purposes, first it serves as a courtesy notification both to the
committer and contributor as to why the commit is being reverted and
second it also ends up being a reminder to the committer that their action
affected the broader community, that they need to be cognizant of it and be
more careful in the future. Also, I am against any public shaming of
individuals in the emails.

Thanks

On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


I think it is a good idea to make the committer responsible for fixing the
situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect the
community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad


On 4/28/17 14:21, Amol Kekre wrote:


Strongly agree with Ilya. Lets take these events as learning opportunities
for folks to learn and improve. There can always be second commit to fix
in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster. We
need to be cognizant of new contributors worrying about the cost to submit
code.

I too do not think Apex is hurting from bad code getting in. We are doing
great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
ilya.gane...@capitalone.com>
wrote:

Guess we can all go home then. Our work here is done:




W.R.T the discussion below, I think rolling back an improperly reviewed
PR
could be considered disrespectful to the committer who merged it in the
first place. I think that such situations, unless they trigger a
disaster,
should be handled by communicating the error to the responsible party and
then allowing them to resolve it. E.g. I improperly commit an unreviewed
PR, someone notices and sends me an email informing me of my error, and I
then have the responsibility of unrolling the change and getting the
appropriate review. I think we should start with the premise that we’re
here in the spirit of collaboration and we should create opportunities
for
individuals to learn from their mistakes, recognize the importance of
particular standards (e.g. good review process leads to stable projects),
and ultimately internalize these ethics.



Internally to our team, we’ve had great success with a policy requiring
two PR approvals and not allowing the creator of a patch to be the one to
merge their PR. While this might feel a little silly, it definitely helps
to build collaboration, familiarity with the code base, and intrinsically
avoids PRs being merged too quickly (without a sufficient period for
review).





- Ilya Ganelin

[image: id:image001.png@01D1F7A4.F3D42980]



*From: *Pramod Immaneni <pra...@datatorrent.com>
*Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
*Date: *Friday, April 28, 2017 at 10:09 AM
*To: *"dev@apex.apache.org"

Re: PR merge policy

2017-04-29 Thread Munagala Ramanath

Given the divergence of views on this topic, I think we need to clarify
what "last resort"
means; to me it means:

In most cases when PRs are prematurely merged, the preferred
mechanism to address it will be to open a new PR with corrections resulting
in a new
commit on top. A forced reversion of the prior commit will only occur only
when it is
judged to be disastrously flawed.

Ram



On Sat, Apr 29, 2017 at 8:33 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> Yes, the rollback is the last resort as Thomas mentioned. I hope that we
> will not need it.
>
> Possibly I misjudge, but I don't see forced push as an extremely
> disturbing event for the community especially if it is done quickly.
>
> Thank you,
>
> Vlad
>
> On 4/28/17 23:33, Bhupesh Chawda wrote:
>
>> 
>> Vlad,
>>
>> Your point regarding not merging any PR without allowing the community to
>> review is well taken.
>> I agree that the committer should be responsible for rolling back the
>> change and
>> I think 
>> we should establish the way in which it should be done.
>>
>> There
>> can
>>   be a delay for the committer to notice that a commit should not have
>> been
>> merged and needs to be reverted. But it might be a while before the
>> committer realizes this and a force push might create problems for the
>> community. Perhaps a new PR could be the way to go.
>>
>> Note that I agree that
>> undoing a PR
>>   may not even be needed given that the community has had a chance to
>> review
>> it, but there might still be cases where such undo commits need to be
>> done.
>>
>> ~ Bhupesh
>>
>>
>> ___
>>
>> Bhupesh Chawda
>>
>> E: bhup...@datatorrent.com | Twitter: @bhupeshsc
>>
>> www.datatorrent.com  |  apex.apache.org
>>
>>
>>
>> On Sat, Apr 29, 2017 at 11:07 AM, Vlad Rozov <v.ro...@datatorrent.com>
>> wrote:
>>
>> If a committer is fast to pull the trigger to merge and slow to rollback
>>> when the policy is violated, the case can be sent to PMC. It will be a
>>> clear indication that a committer does not respect the community, so I
>>> disagree that this is "just kicking the can down the road" as committer
>>> right may be eventually revoked (hope we don't need to go that far ever).
>>>
>>> Not all policies are immediately reflected at
>>> http://apex.apache.org/contributing.html. The vote clearly happened a
>>> week ago when I initially proposed that a PR can be merged only once the
>>> community has a chance to review it . http://apex.apache.org/contrib
>>> uting.html is for new contributors and new committers, existing
>>> committers should be fully aware of the PR merge policy and if in doubt,
>>> raise a question here (dev@apex).
>>>
>>> We are all humans and may overlook problems in PRs that we review. The
>>> concern is not that Travis build may fail and a commit needs to be
>>> reverted. It will not be a valid reason to undo the commit (note that it
>>> is
>>> still committer responsibility to thoroughly review changes/new code and
>>> ensure that license is in place, build is free from compilation and other
>>> errors, code is optimal and readable, and basically put his/her name next
>>> to the contributor name). The concern is when a committer does not give
>>> community a chance for review. It is against "community before code"
>>> behavior that we want to prohibit. I am strongly for requesting a
>>> contributor to undo a commit in such cases and disallowing additional
>>> "fix"
>>> commits.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>> On 4/28/17 20:17, Munagala Ramanath wrote:
>>>
>>> That's just kicking the can down the road: What if the committer is not
>>>> inclined
>>>> to perform the rollback ? Other reviewers can provide feedback on the
>>>> closed PR
>>>> but the committer may choose to add a new commit on top.
>>>>
>>>> Firstly, the point about waiting a day or two is not even a formal
>>>> policy
>>>> requirement
>>>> outlined at http://apex.apache.org/contributing.html in the "Merging a
>>>> Pull
>>>> Request"
>>>> section, so it only has an advisory status currently in my view.
>>>>
>>>> Secondly, I think a variety of so called "violations"

Re: PR merge policy

Yes, the rollback is the last resort as Thomas mentioned. I hope that we 
will not need it.


Possibly I misjudge, but I don't see forced push as an extremely 
disturbing event for the community especially if it is done quickly.


Thank you,

Vlad

On 4/28/17 23:33, Bhupesh Chawda wrote:


Vlad,

Your point regarding not merging any PR without allowing the community to
review is well taken.
I agree that the committer should be responsible for rolling back the
change and
I think 
we should establish the way in which it should be done.

There
can
  be a delay for the committer to notice that a commit should not have been
merged and needs to be reverted. But it might be a while before the
committer realizes this and a force push might create problems for the
community. Perhaps a new PR could be the way to go.

Note that I agree that
undoing a PR
  may not even be needed given that the community has had a chance to review
it, but there might still be cases where such undo commits need to be done.

~ Bhupesh


___

Bhupesh Chawda

E: bhup...@datatorrent.com | Twitter: @bhupeshsc

www.datatorrent.com  |  apex.apache.org



On Sat, Apr 29, 2017 at 11:07 AM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:


If a committer is fast to pull the trigger to merge and slow to rollback
when the policy is violated, the case can be sent to PMC. It will be a
clear indication that a committer does not respect the community, so I
disagree that this is "just kicking the can down the road" as committer
right may be eventually revoked (hope we don't need to go that far ever).

Not all policies are immediately reflected at
http://apex.apache.org/contributing.html. The vote clearly happened a
week ago when I initially proposed that a PR can be merged only once the
community has a chance to review it . http://apex.apache.org/contrib
uting.html is for new contributors and new committers, existing
committers should be fully aware of the PR merge policy and if in doubt,
raise a question here (dev@apex).

We are all humans and may overlook problems in PRs that we review. The
concern is not that Travis build may fail and a commit needs to be
reverted. It will not be a valid reason to undo the commit (note that it is
still committer responsibility to thoroughly review changes/new code and
ensure that license is in place, build is free from compilation and other
errors, code is optimal and readable, and basically put his/her name next
to the contributor name). The concern is when a committer does not give
community a chance for review. It is against "community before code"
behavior that we want to prohibit. I am strongly for requesting a
contributor to undo a commit in such cases and disallowing additional "fix"
commits.

Thank you,

Vlad

On 4/28/17 20:17, Munagala Ramanath wrote:


That's just kicking the can down the road: What if the committer is not
inclined
to perform the rollback ? Other reviewers can provide feedback on the
closed PR
but the committer may choose to add a new commit on top.

Firstly, the point about waiting a day or two is not even a formal policy
requirement
outlined at http://apex.apache.org/contributing.html in the "Merging a
Pull
Request"
section, so it only has an advisory status currently in my view.

Secondly, I think a variety of so called "violations" from overlooking
Travis build
failures, to not detecting missing unit tests, to not enforcing JavaDoc
requirements
are not fatal in most cases. Reverting commits in such cases is contrary
to
the
Apache injunction to "Put community before code" (
http://community.apache.org/newbiefaq.html)

So I am firmly against reverting except in the most dire circumstances and
in favor
of adding new commits to fix issues -- we do that for bugs, no reason to
treat these
other cases differently.

Ram

On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <a...@datatorrent.com> wrote:

That makes sense. The committer should fix upon feedback. PR policy

violation is bad, I am not defending the violation.  think if the
committer
takes the feedback and promptly works on a fix (new pr) it should be ok.

Thks,
Amol



E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I think it is a good idea to make the committer responsible for fixing
the


situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect
the
community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad

On 4/28/17 14:21, Amol Kekre wrote:

Strongly agree with Ilya. Lets take these events as learning
opportunities
for folks to learn and improve. There can

Re: PR merge policy

2017-04-29 Thread Pramod Immaneni

I assume we are still referring to force push and removing the commit from
the upstream git commit tree as rollback and not to applying a new commit
that reverts the changes. When a violation happens, why should everyone
(who synced with the repo) suffer the inconvenience of their local git
repo/branch ending up with a bad git commit state for a problem they did
not cause, which would happen when rollback is employed, when this can be
easily fixed by a new commit that reverts the changes. Second, a violation
while bad, should the default policy be, to immediately revert the changes
with a new PR/commit without looking at what they are? That seems
unnecessarily draconian too, considering that violations are not the norm
nor flagrant that drastic measures need to be taken now. If the changes are
questionable or even simply require more time for review, yes, by all
means, send an email to dev list with the reason for reverting the PR,
revert the changes with a new commit and redo the PR process. The email
serves two purposes, first it serves as a courtesy notification both to the
committer and contributor as to why the commit is being reverted and
second it also ends up being a reminder to the committer that their action
affected the broader community, that they need to be cognizant of it and be
more careful in the future. Also, I am against any public shaming of
individuals in the emails.

Thanks

On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> I think it is a good idea to make the committer responsible for fixing the
> situation by rolling back the commit and re-opening the PR for further
> review. IMO, committer right comes with the responsibility to respect the
> community and policies it established.
>
> I would disagree that rolling back should be used only in case of a
> disaster unless PR merge policy violation is a disaster :-) (and it
> actually is).
>
> Thank you,
>
> Vlad
>
>
> On 4/28/17 14:21, Amol Kekre wrote:
>
>> Strongly agree with Ilya. Lets take these events as learning opportunities
>> for folks to learn and improve. There can always be second commit to fix
>> in
>> case there is code issue. If it is a policy issue, we learn and improve.
>> Rolling back, should be used rarely and it does need to be a disaster. We
>> need to be cognizant of new contributors worrying about the cost to submit
>> code.
>>
>> I too do not think Apex is hurting from bad code getting in. We are doing
>> great with our current policies.
>>
>> Thks,
>> Amol
>>
>>
>> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>>
>> www.datatorrent.com
>>
>>
>> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
>> ilya.gane...@capitalone.com>
>> wrote:
>>
>> Guess we can all go home then. Our work here is done:
>>>
>>>
>>>
>>>
>>> W.R.T the discussion below, I think rolling back an improperly reviewed
>>> PR
>>> could be considered disrespectful to the committer who merged it in the
>>> first place. I think that such situations, unless they trigger a
>>> disaster,
>>> should be handled by communicating the error to the responsible party and
>>> then allowing them to resolve it. E.g. I improperly commit an unreviewed
>>> PR, someone notices and sends me an email informing me of my error, and I
>>> then have the responsibility of unrolling the change and getting the
>>> appropriate review. I think we should start with the premise that we’re
>>> here in the spirit of collaboration and we should create opportunities
>>> for
>>> individuals to learn from their mistakes, recognize the importance of
>>> particular standards (e.g. good review process leads to stable projects),
>>> and ultimately internalize these ethics.
>>>
>>>
>>>
>>> Internally to our team, we’ve had great success with a policy requiring
>>> two PR approvals and not allowing the creator of a patch to be the one to
>>> merge their PR. While this might feel a little silly, it definitely helps
>>> to build collaboration, familiarity with the code base, and intrinsically
>>> avoids PRs being merged too quickly (without a sufficient period for
>>> review).
>>>
>>>
>>>
>>>
>>>
>>> - Ilya Ganelin
>>>
>>> [image: id:image001.png@01D1F7A4.F3D42980]
>>>
>>>
>>>
>>> *From: *Pramod Immaneni <pra...@datatorrent.com>
>>> *Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
>>> *Date: *Friday, April 28, 2017 at 10:09 A

Re: PR merge policy

2017-04-29 Thread Bhupesh Chawda


Vlad,

Your point regarding not merging any PR without allowing the community to
review is well taken.
I agree that the committer should be responsible for rolling back the
change and
I think 
we should establish the way in which it should be done.

There
can
 be a delay for the committer to notice that a commit should not have been
merged and needs to be reverted. But it might be a while before the
committer realizes this and a force push might create problems for the
community. Perhaps a new PR could be the way to go.

Note that I agree that
undoing a PR
 may not even be needed given that the community has had a chance to review
it, but there might still be cases where such undo commits need to be done.

~ Bhupesh


___

Bhupesh Chawda

E: bhup...@datatorrent.com | Twitter: @bhupeshsc

www.datatorrent.com  |  apex.apache.org



On Sat, Apr 29, 2017 at 11:07 AM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

> If a committer is fast to pull the trigger to merge and slow to rollback
> when the policy is violated, the case can be sent to PMC. It will be a
> clear indication that a committer does not respect the community, so I
> disagree that this is "just kicking the can down the road" as committer
> right may be eventually revoked (hope we don't need to go that far ever).
>
> Not all policies are immediately reflected at
> http://apex.apache.org/contributing.html. The vote clearly happened a
> week ago when I initially proposed that a PR can be merged only once the
> community has a chance to review it . http://apex.apache.org/contrib
> uting.html is for new contributors and new committers, existing
> committers should be fully aware of the PR merge policy and if in doubt,
> raise a question here (dev@apex).
>
> We are all humans and may overlook problems in PRs that we review. The
> concern is not that Travis build may fail and a commit needs to be
> reverted. It will not be a valid reason to undo the commit (note that it is
> still committer responsibility to thoroughly review changes/new code and
> ensure that license is in place, build is free from compilation and other
> errors, code is optimal and readable, and basically put his/her name next
> to the contributor name). The concern is when a committer does not give
> community a chance for review. It is against "community before code"
> behavior that we want to prohibit. I am strongly for requesting a
> contributor to undo a commit in such cases and disallowing additional "fix"
> commits.
>
> Thank you,
>
> Vlad
>
> On 4/28/17 20:17, Munagala Ramanath wrote:
>
>> That's just kicking the can down the road: What if the committer is not
>> inclined
>> to perform the rollback ? Other reviewers can provide feedback on the
>> closed PR
>> but the committer may choose to add a new commit on top.
>>
>> Firstly, the point about waiting a day or two is not even a formal policy
>> requirement
>> outlined at http://apex.apache.org/contributing.html in the "Merging a
>> Pull
>> Request"
>> section, so it only has an advisory status currently in my view.
>>
>> Secondly, I think a variety of so called "violations" from overlooking
>> Travis build
>> failures, to not detecting missing unit tests, to not enforcing JavaDoc
>> requirements
>> are not fatal in most cases. Reverting commits in such cases is contrary
>> to
>> the
>> Apache injunction to "Put community before code" (
>> http://community.apache.org/newbiefaq.html)
>>
>> So I am firmly against reverting except in the most dire circumstances and
>> in favor
>> of adding new commits to fix issues -- we do that for bugs, no reason to
>> treat these
>> other cases differently.
>>
>> Ram
>>
>> On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <a...@datatorrent.com> wrote:
>>
>> That makes sense. The committer should fix upon feedback. PR policy
>>> violation is bad, I am not defending the violation.  think if the
>>> committer
>>> takes the feedback and promptly works on a fix (new pr) it should be ok.
>>>
>>> Thks,
>>> Amol
>>>
>>>
>>>
>>> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>>>
>>> www.datatorrent.com
>>>
>>>
>>> On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>> wrote:
>>>
>>> I think it is a good idea to make the committer responsible for fixing
>>>>
>>> the
>>>
>>>> situation by rolling back the commit and re-opening the PR for fur

Re: PR merge policy

2017-04-28 Thread Vlad Rozov

If a committer is fast to pull the trigger to merge and slow to rollback 
when the policy is violated, the case can be sent to PMC. It will be a 
clear indication that a committer does not respect the community, so I 
disagree that this is "just kicking the can down the road" as committer 
right may be eventually revoked (hope we don't need to go that far ever).


Not all policies are immediately reflected at 
http://apex.apache.org/contributing.html. The vote clearly happened a 
week ago when I initially proposed that a PR can be merged only once the 
community has a chance to review it . 
http://apex.apache.org/contributing.html is for new contributors and new 
committers, existing committers should be fully aware of the PR merge 
policy and if in doubt, raise a question here (dev@apex).


We are all humans and may overlook problems in PRs that we review. The 
concern is not that Travis build may fail and a commit needs to be 
reverted. It will not be a valid reason to undo the commit (note that it 
is still committer responsibility to thoroughly review changes/new code 
and ensure that license is in place, build is free from compilation and 
other errors, code is optimal and readable, and basically put his/her 
name next to the contributor name). The concern is when a committer does 
not give community a chance for review. It is against "community before 
code" behavior that we want to prohibit. I am strongly for requesting a 
contributor to undo a commit in such cases and disallowing additional 
"fix" commits.


Thank you,

Vlad
On 4/28/17 20:17, Munagala Ramanath wrote:

That's just kicking the can down the road: What if the committer is not
inclined
to perform the rollback ? Other reviewers can provide feedback on the
closed PR
but the committer may choose to add a new commit on top.

Firstly, the point about waiting a day or two is not even a formal policy
requirement
outlined at http://apex.apache.org/contributing.html in the "Merging a Pull
Request"
section, so it only has an advisory status currently in my view.

Secondly, I think a variety of so called "violations" from overlooking
Travis build
failures, to not detecting missing unit tests, to not enforcing JavaDoc
requirements
are not fatal in most cases. Reverting commits in such cases is contrary to
the
Apache injunction to "Put community before code" (
http://community.apache.org/newbiefaq.html)

So I am firmly against reverting except in the most dire circumstances and
in favor
of adding new commits to fix issues -- we do that for bugs, no reason to
treat these
other cases differently.

Ram

On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <a...@datatorrent.com> wrote:


That makes sense. The committer should fix upon feedback. PR policy
violation is bad, I am not defending the violation.  think if the committer
takes the feedback and promptly works on a fix (new pr) it should be ok.

Thks,
Amol



E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:


I think it is a good idea to make the committer responsible for fixing

the

situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect the
community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad

On 4/28/17 14:21, Amol Kekre wrote:


Strongly agree with Ilya. Lets take these events as learning

opportunities

for folks to learn and improve. There can always be second commit to fix
in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster.

We

need to be cognizant of new contributors worrying about the cost to

submit

code.

I too do not think Apex is hurting from bad code getting in. We are

doing

great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
ilya.gane...@capitalone.com>
wrote:

Guess we can all go home then. Our work here is done:




W.R.T the discussion below, I think rolling back an improperly reviewed
PR
could be considered disrespectful to the committer who merged it in the
first place. I think that such situations, unless they trigger a
disaster,
should be handled by communicating the error to the responsible party

and

then allowing them to resolve it. E.g. I improperly commit an

unreviewed

PR, someone notices and sends me an email informing me of my error,

and I

then have the responsibility of unrolling the change and getting the
appropriate review. I think we should start with the premise that we’re
here in the spirit of collaboration a

Re: PR merge policy

2017-04-28 Thread Munagala Ramanath

That's just kicking the can down the road: What if the committer is not
inclined
to perform the rollback ? Other reviewers can provide feedback on the
closed PR
but the committer may choose to add a new commit on top.

Firstly, the point about waiting a day or two is not even a formal policy
requirement
outlined at http://apex.apache.org/contributing.html in the "Merging a Pull
Request"
section, so it only has an advisory status currently in my view.

Secondly, I think a variety of so called "violations" from overlooking
Travis build
failures, to not detecting missing unit tests, to not enforcing JavaDoc
requirements
are not fatal in most cases. Reverting commits in such cases is contrary to
the
Apache injunction to "Put community before code" (
http://community.apache.org/newbiefaq.html)

So I am firmly against reverting except in the most dire circumstances and
in favor
of adding new commits to fix issues -- we do that for bugs, no reason to
treat these
other cases differently.

Ram

On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <a...@datatorrent.com> wrote:

> That makes sense. The committer should fix upon feedback. PR policy
> violation is bad, I am not defending the violation.  think if the committer
> takes the feedback and promptly works on a fix (new pr) it should be ok.
>
> Thks,
> Amol
>
>
>
> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>
> www.datatorrent.com
>
>
> On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> > I think it is a good idea to make the committer responsible for fixing
> the
> > situation by rolling back the commit and re-opening the PR for further
> > review. IMO, committer right comes with the responsibility to respect the
> > community and policies it established.
> >
> > I would disagree that rolling back should be used only in case of a
> > disaster unless PR merge policy violation is a disaster :-) (and it
> > actually is).
> >
> > Thank you,
> >
> > Vlad
> >
> > On 4/28/17 14:21, Amol Kekre wrote:
> >
> >> Strongly agree with Ilya. Lets take these events as learning
> opportunities
> >> for folks to learn and improve. There can always be second commit to fix
> >> in
> >> case there is code issue. If it is a policy issue, we learn and improve.
> >> Rolling back, should be used rarely and it does need to be a disaster.
> We
> >> need to be cognizant of new contributors worrying about the cost to
> submit
> >> code.
> >>
> >> I too do not think Apex is hurting from bad code getting in. We are
> doing
> >> great with our current policies.
> >>
> >> Thks,
> >> Amol
> >>
> >>
> >> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
> >>
> >> www.datatorrent.com
> >>
> >>
> >> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
> >> ilya.gane...@capitalone.com>
> >> wrote:
> >>
> >> Guess we can all go home then. Our work here is done:
> >>>
> >>>
> >>>
> >>>
> >>> W.R.T the discussion below, I think rolling back an improperly reviewed
> >>> PR
> >>> could be considered disrespectful to the committer who merged it in the
> >>> first place. I think that such situations, unless they trigger a
> >>> disaster,
> >>> should be handled by communicating the error to the responsible party
> and
> >>> then allowing them to resolve it. E.g. I improperly commit an
> unreviewed
> >>> PR, someone notices and sends me an email informing me of my error,
> and I
> >>> then have the responsibility of unrolling the change and getting the
> >>> appropriate review. I think we should start with the premise that we’re
> >>> here in the spirit of collaboration and we should create opportunities
> >>> for
> >>> individuals to learn from their mistakes, recognize the importance of
> >>> particular standards (e.g. good review process leads to stable
> projects),
> >>> and ultimately internalize these ethics.
> >>>
> >>>
> >>>
> >>> Internally to our team, we’ve had great success with a policy requiring
> >>> two PR approvals and not allowing the creator of a patch to be the one
> to
> >>> merge their PR. While this might feel a little silly, it definitely
> helps
> >>> to build collaboration, familiarity with the code base, and
> intrinsically
> >>> avoids PRs being merged to

Re: PR merge policy

2017-04-28 Thread Vlad Rozov

I don't think we should formalize the requirement to have at least 2 
sign-off on a PR before it is merged, but at the same time I would 
encourage more contributors and committers to review PRs. This will 
bring more collaboration and familiarity with how Apex works that Ilya 
mentioned and also will speedup PR review and improve quality of commits.


On a side note, we do allow committers to commit their own PRs after 
other committers sign-off on it. It is done to avoid empty merge 
commits. When reviewer and committer are part of the same organization 
and can coordinate proper rebase, committing own PR should be avoided.


Thank you,

Vlad

On 4/28/17 13:35, Ganelin, Ilya wrote:


Guess we can all go home then. Our work here is done:


W.R.T the discussion below, I think rolling back an improperly 
reviewed PR could be considered disrespectful to the committer who 
merged it in the first place. I think that such situations, unless 
they trigger a disaster, should be handled by communicating the error 
to the responsible party and then allowing them to resolve it. E.g. I 
improperly commit an unreviewed PR, someone notices and sends me an 
email informing me of my error, and I then have the responsibility of 
unrolling the change and getting the appropriate review. I think we 
should start with the premise that we’re here in the spirit of 
collaboration and we should create opportunities for individuals to 
learn from their mistakes, recognize the importance of particular 
standards (e.g. good review process leads to stable projects), and 
ultimately internalize these ethics.


Internally to our team, we’ve had great success with a policy 
requiring two PR approvals and not allowing the creator of a patch to 
be the one to merge their PR. While this might feel a little silly, it 
definitely helps to build collaboration, familiarity with the code 
base, and intrinsically avoids PRs being merged too quickly (without a 
sufficient period for review).


- Ilya Ganelin

id:image001.png@01D1F7A4.F3D42980

*From: *Pramod Immaneni <pra...@datatorrent.com>
*Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
*Date: *Friday, April 28, 2017 at 10:09 AM
*To: *"dev@apex.apache.org" <dev@apex.apache.org>
*Subject: *Re: PR merge policy

On a lighter note, looks like the powers that be have been listening 
on this conversation and decided to force push an empty repo or maybe 
github just decided that this is the best proposal ;)


On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.ro...@datatorrent.com 
<mailto:v.ro...@datatorrent.com>> wrote:


    In this case please propose how to deal with PR merge policy
violations in the future. I will -1 proposal to commit an
improvement on top of a commit.

Thank you,

Vlad



On 4/27/17 21:48, Pramod Immaneni wrote:

I am sorry but I am -1 on the force push in this case.

On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org
<mailto:t...@apache.org>> wrote:

+1 as measure of last resort.

On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov
<v.ro...@datatorrent.com
<mailto:v.ro...@datatorrent.com>> wrote:

IMO, force push will bring enough consequent
embarrassment to avoid such
behavior in the future.

Thank you,

Vlad

On 4/27/17 21:16, Munagala Ramanath wrote:

My thought was that leaving the bad commit would
be a permanent reminder
to
the committer
(and others) that a policy violation occurred and
the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov
<v.ro...@datatorrent.com
<mailto:v.ro...@datatorrent.com>>
wrote:

I also was under impression that everyone agreed
to the policy that gives

everyone in the community a chance to raise a
concern or to propose an
improvement to a PR. Unfortunately, it is not
the case, and we need to
discuss it again. I hope that this discussion
will lead to no future
violations so we don't need to forcibly undo
such commits, but it will be
good for the community to agree on the policy
that deals with violations.

Ram, committing an improvement on top of a
commit should be discouraged,
not encou

Re: PR merge policy

2017-04-28 Thread Amol Kekre

That makes sense. The committer should fix upon feedback. PR policy
violation is bad, I am not defending the violation.  think if the committer
takes the feedback and promptly works on a fix (new pr) it should be ok.

Thks,
Amol



E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> I think it is a good idea to make the committer responsible for fixing the
> situation by rolling back the commit and re-opening the PR for further
> review. IMO, committer right comes with the responsibility to respect the
> community and policies it established.
>
> I would disagree that rolling back should be used only in case of a
> disaster unless PR merge policy violation is a disaster :-) (and it
> actually is).
>
> Thank you,
>
> Vlad
>
> On 4/28/17 14:21, Amol Kekre wrote:
>
>> Strongly agree with Ilya. Lets take these events as learning opportunities
>> for folks to learn and improve. There can always be second commit to fix
>> in
>> case there is code issue. If it is a policy issue, we learn and improve.
>> Rolling back, should be used rarely and it does need to be a disaster. We
>> need to be cognizant of new contributors worrying about the cost to submit
>> code.
>>
>> I too do not think Apex is hurting from bad code getting in. We are doing
>> great with our current policies.
>>
>> Thks,
>> Amol
>>
>>
>> E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>>
>> www.datatorrent.com
>>
>>
>> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
>> ilya.gane...@capitalone.com>
>> wrote:
>>
>> Guess we can all go home then. Our work here is done:
>>>
>>>
>>>
>>>
>>> W.R.T the discussion below, I think rolling back an improperly reviewed
>>> PR
>>> could be considered disrespectful to the committer who merged it in the
>>> first place. I think that such situations, unless they trigger a
>>> disaster,
>>> should be handled by communicating the error to the responsible party and
>>> then allowing them to resolve it. E.g. I improperly commit an unreviewed
>>> PR, someone notices and sends me an email informing me of my error, and I
>>> then have the responsibility of unrolling the change and getting the
>>> appropriate review. I think we should start with the premise that we’re
>>> here in the spirit of collaboration and we should create opportunities
>>> for
>>> individuals to learn from their mistakes, recognize the importance of
>>> particular standards (e.g. good review process leads to stable projects),
>>> and ultimately internalize these ethics.
>>>
>>>
>>>
>>> Internally to our team, we’ve had great success with a policy requiring
>>> two PR approvals and not allowing the creator of a patch to be the one to
>>> merge their PR. While this might feel a little silly, it definitely helps
>>> to build collaboration, familiarity with the code base, and intrinsically
>>> avoids PRs being merged too quickly (without a sufficient period for
>>> review).
>>>
>>>
>>>
>>>
>>>
>>> - Ilya Ganelin
>>>
>>> [image: id:image001.png@01D1F7A4.F3D42980]
>>>
>>>
>>>
>>> *From: *Pramod Immaneni <pra...@datatorrent.com>
>>> *Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
>>> *Date: *Friday, April 28, 2017 at 10:09 AM
>>> *To: *"dev@apex.apache.org" <dev@apex.apache.org>
>>> *Subject: *Re: PR merge policy
>>>
>>>
>>>
>>>
>>> On a lighter note, looks like the powers that be have been listening on
>>> this conversation and decided to force push an empty repo or maybe
>>> github just decided that this is the best proposal ;)
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>> wrote:
>>>
>>> In this case please propose how to deal with PR merge policy violations
>>> in
>>> the future. I will -1 proposal to commit an improvement on top of a
>>> commit.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>>
>>>
>>> On 4/27/17 21:48, Pramod Immaneni wrote:
>>>
>>> I am sorry but I am -1 on the force push in this case.
>>>
>>> On Apr 27, 2017, at

Re: PR merge policy

2017-04-28 Thread Vlad Rozov

I think it is a good idea to make the committer responsible for fixing 
the situation by rolling back the commit and re-opening the PR for 
further review. IMO, committer right comes with the responsibility to 
respect the community and policies it established.


I would disagree that rolling back should be used only in case of a 
disaster unless PR merge policy violation is a disaster :-) (and it 
actually is).


Thank you,

Vlad

On 4/28/17 14:21, Amol Kekre wrote:

Strongly agree with Ilya. Lets take these events as learning opportunities
for folks to learn and improve. There can always be second commit to fix in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster. We
need to be cognizant of new contributors worrying about the cost to submit
code.

I too do not think Apex is hurting from bad code getting in. We are doing
great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <ilya.gane...@capitalone.com>
wrote:


Guess we can all go home then. Our work here is done:




W.R.T the discussion below, I think rolling back an improperly reviewed PR
could be considered disrespectful to the committer who merged it in the
first place. I think that such situations, unless they trigger a disaster,
should be handled by communicating the error to the responsible party and
then allowing them to resolve it. E.g. I improperly commit an unreviewed
PR, someone notices and sends me an email informing me of my error, and I
then have the responsibility of unrolling the change and getting the
appropriate review. I think we should start with the premise that we’re
here in the spirit of collaboration and we should create opportunities for
individuals to learn from their mistakes, recognize the importance of
particular standards (e.g. good review process leads to stable projects),
and ultimately internalize these ethics.



Internally to our team, we’ve had great success with a policy requiring
two PR approvals and not allowing the creator of a patch to be the one to
merge their PR. While this might feel a little silly, it definitely helps
to build collaboration, familiarity with the code base, and intrinsically
avoids PRs being merged too quickly (without a sufficient period for
review).





- Ilya Ganelin

[image: id:image001.png@01D1F7A4.F3D42980]



*From: *Pramod Immaneni <pra...@datatorrent.com>
*Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
*Date: *Friday, April 28, 2017 at 10:09 AM
*To: *"dev@apex.apache.org" <dev@apex.apache.org>
*Subject: *Re: PR merge policy



On a lighter note, looks like the powers that be have been listening on
this conversation and decided to force push an empty repo or maybe
github just decided that this is the best proposal ;)







On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

In this case please propose how to deal with PR merge policy violations in
the future. I will -1 proposal to commit an improvement on top of a commit.

Thank you,

Vlad



On 4/27/17 21:48, Pramod Immaneni wrote:

I am sorry but I am -1 on the force push in this case.

On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org> wrote:

+1 as measure of last resort.

On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

IMO, force push will bring enough consequent embarrassment to avoid such
behavior in the future.

Thank you,

Vlad

On 4/27/17 21:16, Munagala Ramanath wrote:

My thought was that leaving the bad commit would be a permanent reminder
to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I also was under impression that everyone agreed to the policy that gives

everyone in the community a chance to raise a concern or to propose an
improvement to a PR. Unfortunately, it is not the case, and we need to
discuss it again. I hope that this discussion will lead to no future
violations so we don't need to forcibly undo such commits, but it will be
good for the community to agree on the policy that deals with violations.

Ram, committing an improvement on top of a commit should be discouraged,
not encouraged as it eventually leads to the policy violation and lousy
PR
reviews.

Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:

I also thought that everybody was in agreement about that after the first

round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation,
I'm
more interested in a style of collaboration that does not require such
discus

Re: PR merge policy

2017-04-28 Thread Amol Kekre

Strongly agree with Ilya. Lets take these events as learning opportunities
for folks to learn and improve. There can always be second commit to fix in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster. We
need to be cognizant of new contributors worrying about the cost to submit
code.

I too do not think Apex is hurting from bad code getting in. We are doing
great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <ilya.gane...@capitalone.com>
wrote:

> Guess we can all go home then. Our work here is done:
>
>
>
>
> W.R.T the discussion below, I think rolling back an improperly reviewed PR
> could be considered disrespectful to the committer who merged it in the
> first place. I think that such situations, unless they trigger a disaster,
> should be handled by communicating the error to the responsible party and
> then allowing them to resolve it. E.g. I improperly commit an unreviewed
> PR, someone notices and sends me an email informing me of my error, and I
> then have the responsibility of unrolling the change and getting the
> appropriate review. I think we should start with the premise that we’re
> here in the spirit of collaboration and we should create opportunities for
> individuals to learn from their mistakes, recognize the importance of
> particular standards (e.g. good review process leads to stable projects),
> and ultimately internalize these ethics.
>
>
>
> Internally to our team, we’ve had great success with a policy requiring
> two PR approvals and not allowing the creator of a patch to be the one to
> merge their PR. While this might feel a little silly, it definitely helps
> to build collaboration, familiarity with the code base, and intrinsically
> avoids PRs being merged too quickly (without a sufficient period for
> review).
>
>
>
>
>
> - Ilya Ganelin
>
> [image: id:image001.png@01D1F7A4.F3D42980]
>
>
>
> *From: *Pramod Immaneni <pra...@datatorrent.com>
> *Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
> *Date: *Friday, April 28, 2017 at 10:09 AM
> *To: *"dev@apex.apache.org" <dev@apex.apache.org>
> *Subject: *Re: PR merge policy
>
>
>
> On a lighter note, looks like the powers that be have been listening on
> this conversation and decided to force push an empty repo or maybe
> github just decided that this is the best proposal ;)
>
>
>
>
>
>
>
> On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> In this case please propose how to deal with PR merge policy violations in
> the future. I will -1 proposal to commit an improvement on top of a commit.
>
> Thank you,
>
> Vlad
>
>
>
> On 4/27/17 21:48, Pramod Immaneni wrote:
>
> I am sorry but I am -1 on the force push in this case.
>
> On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org> wrote:
>
> +1 as measure of last resort.
>
> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> IMO, force push will bring enough consequent embarrassment to avoid such
> behavior in the future.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 21:16, Munagala Ramanath wrote:
>
> My thought was that leaving the bad commit would be a permanent reminder
> to
> the committer
> (and others) that a policy violation occurred and the consequent
> embarrassment would be an
> adequate deterrent.
>
> Ram
>
> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> I also was under impression that everyone agreed to the policy that gives
>
> everyone in the community a chance to raise a concern or to propose an
> improvement to a PR. Unfortunately, it is not the case, and we need to
> discuss it again. I hope that this discussion will lead to no future
> violations so we don't need to forcibly undo such commits, but it will be
> good for the community to agree on the policy that deals with violations.
>
> Ram, committing an improvement on top of a commit should be discouraged,
> not encouraged as it eventually leads to the policy violation and lousy
> PR
> reviews.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 20:54, Thomas Weise wrote:
>
> I also thought that everybody was in agreement about that after the first
>
> round of discussion and as you say it would be hard to argue against it.
> And I think we should not have to be back to the same topic a few days
> later.
>
> While you seem to be focussed on the disagreement on p

Re: PR merge policy

2017-04-28 Thread Ganelin, Ilya

Guess we can all go home then. Our work here is done:

[cid:image001.png@01D2C024.5F2A5090]

W.R.T the discussion below, I think rolling back an improperly reviewed PR 
could be considered disrespectful to the committer who merged it in the first 
place. I think that such situations, unless they trigger a disaster, should be 
handled by communicating the error to the responsible party and then allowing 
them to resolve it. E.g. I improperly commit an unreviewed PR, someone notices 
and sends me an email informing me of my error, and I then have the 
responsibility of unrolling the change and getting the appropriate review. I 
think we should start with the premise that we’re here in the spirit of 
collaboration and we should create opportunities for individuals to learn from 
their mistakes, recognize the importance of particular standards (e.g. good 
review process leads to stable projects), and ultimately internalize these 
ethics.

Internally to our team, we’ve had great success with a policy requiring two PR 
approvals and not allowing the creator of a patch to be the one to merge their 
PR. While this might feel a little silly, it definitely helps to build 
collaboration, familiarity with the code base, and intrinsically avoids PRs 
being merged too quickly (without a sufficient period for review).


- Ilya Ganelin
[id:image001.png@01D1F7A4.F3D42980]

From: Pramod Immaneni <pra...@datatorrent.com>
Reply-To: "dev@apex.apache.org" <dev@apex.apache.org>
Date: Friday, April 28, 2017 at 10:09 AM
To: "dev@apex.apache.org" <dev@apex.apache.org>
Subject: Re: PR merge policy

On a lighter note, looks like the powers that be have been listening on this 
conversation and decided to force push an empty repo or maybe github just 
decided that this is the best proposal ;)



On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov 
<v.ro...@datatorrent.com<mailto:v.ro...@datatorrent.com>> wrote:
In this case please propose how to deal with PR merge policy violations in the 
future. I will -1 proposal to commit an improvement on top of a commit.

Thank you,

Vlad


On 4/27/17 21:48, Pramod Immaneni wrote:
I am sorry but I am -1 on the force push in this case.
On Apr 27, 2017, at 9:27 PM, Thomas Weise 
<t...@apache.org<mailto:t...@apache.org>> wrote:

+1 as measure of last resort.
On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov 
<v.ro...@datatorrent.com<mailto:v.ro...@datatorrent.com>> wrote:

IMO, force push will bring enough consequent embarrassment to avoid such
behavior in the future.

Thank you,

Vlad
On 4/27/17 21:16, Munagala Ramanath wrote:

My thought was that leaving the bad commit would be a permanent reminder
to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov 
<v.ro...@datatorrent.com<mailto:v.ro...@datatorrent.com>>
wrote:

I also was under impression that everyone agreed to the policy that gives
everyone in the community a chance to raise a concern or to propose an
improvement to a PR. Unfortunately, it is not the case, and we need to
discuss it again. I hope that this discussion will lead to no future
violations so we don't need to forcibly undo such commits, but it will be
good for the community to agree on the policy that deals with violations.

Ram, committing an improvement on top of a commit should be discouraged,
not encouraged as it eventually leads to the policy violation and lousy
PR
reviews.

Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:

I also thought that everybody was in agreement about that after the first
round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation,
I'm
more interested in a style of collaboration that does not require such
discussion.

Thomas

On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath 
<r...@datatorrent.com<mailto:r...@datatorrent.com>
wrote:

Everybody seems agreed on what the committers should do -- that waiting
a
day or two for
others to have a chance to comment seems like an entirely reasonable
thing.

The disagreement is about what to do when that policy is violated.

Ram

On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise 
<t...@apache.org<mailto:t...@apache.org>> wrote:

Forced push should not be necessary if committers follow the
development
process.

So why not focus on what the committer should do? Development process
and
guidelines are there for a reason, and the issue was raised before.

In this specific case, there is a string of commits related to the
plugin
feature that IMO should be part of the original review. There wasn't
any
need to rush this, the change wasn't important for the release.

Thomas


On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
r...@data

Re: PR merge policy

2017-04-28 Thread Pramod Immaneni

On a lighter note, looks like the powers that be have been listening on
this conversation and decided to force push an empty repo or maybe
github just decided that this is the best proposal ;)



On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

> In this case please propose how to deal with PR merge policy violations in
> the future. I will -1 proposal to commit an improvement on top of a commit.
>
> Thank you,
>
> Vlad
>
>
> On 4/27/17 21:48, Pramod Immaneni wrote:
>
>> I am sorry but I am -1 on the force push in this case.
>>
>> On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org> wrote:
>>>
>>> +1 as measure of last resort.
>>>
>>> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>>> wrote:
>>>>
>>>> IMO, force push will bring enough consequent embarrassment to avoid such
>>>> behavior in the future.
>>>>
>>>> Thank you,
>>>>
>>>> Vlad
>>>>
>>>> On 4/27/17 21:16, Munagala Ramanath wrote:
>>>>>
>>>>> My thought was that leaving the bad commit would be a permanent
>>>>> reminder
>>>>> to
>>>>> the committer
>>>>> (and others) that a policy violation occurred and the consequent
>>>>> embarrassment would be an
>>>>> adequate deterrent.
>>>>>
>>>>> Ram
>>>>>
>>>>> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>>>> wrote:
>>>>>
>>>>> I also was under impression that everyone agreed to the policy that
>>>>> gives
>>>>>
>>>>>> everyone in the community a chance to raise a concern or to propose an
>>>>>> improvement to a PR. Unfortunately, it is not the case, and we need to
>>>>>> discuss it again. I hope that this discussion will lead to no future
>>>>>> violations so we don't need to forcibly undo such commits, but it
>>>>>> will be
>>>>>> good for the community to agree on the policy that deals with
>>>>>> violations.
>>>>>>
>>>>>> Ram, committing an improvement on top of a commit should be
>>>>>> discouraged,
>>>>>> not encouraged as it eventually leads to the policy violation and
>>>>>> lousy
>>>>>> PR
>>>>>> reviews.
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Vlad
>>>>>>
>>>>>> On 4/27/17 20:54, Thomas Weise wrote:
>>>>>>
>>>>>> I also thought that everybody was in agreement about that after the
>>>>>> first
>>>>>>
>>>>>>> round of discussion and as you say it would be hard to argue against
>>>>>>> it.
>>>>>>> And I think we should not have to be back to the same topic a few
>>>>>>> days
>>>>>>> later.
>>>>>>>
>>>>>>> While you seem to be focussed on the disagreement on policy
>>>>>>> violation,
>>>>>>> I'm
>>>>>>> more interested in a style of collaboration that does not require
>>>>>>> such
>>>>>>> discussion.
>>>>>>>
>>>>>>> Thomas
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <
>>>>>>> r...@datatorrent.com
>>>>>>> wrote:
>>>>>>>
>>>>>>> Everybody seems agreed on what the committers should do -- that
>>>>>>> waiting
>>>>>>> a
>>>>>>>
>>>>>>> day or two for
>>>>>>>> others to have a chance to comment seems like an entirely reasonable
>>>>>>>> thing.
>>>>>>>>
>>>>>>>> The disagreement is about what to do when that policy is violated.
>>>>>>>>
>>>>>>>> Ram
>>>>>>>>
>>>>>>>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Forced push should not be necessary if committers f

Re: PR merge policy

In this case please propose how to deal with PR merge policy violations 
in the future. I will -1 proposal to commit an improvement on top of a 
commit.


Thank you,

Vlad

On 4/27/17 21:48, Pramod Immaneni wrote:

I am sorry but I am -1 on the force push in this case.


On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org> wrote:

+1 as measure of last resort.


On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

IMO, force push will bring enough consequent embarrassment to avoid such
behavior in the future.

Thank you,

Vlad


On 4/27/17 21:16, Munagala Ramanath wrote:

My thought was that leaving the bad commit would be a permanent reminder
to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I also was under impression that everyone agreed to the policy that gives

everyone in the community a chance to raise a concern or to propose an
improvement to a PR. Unfortunately, it is not the case, and we need to
discuss it again. I hope that this discussion will lead to no future
violations so we don't need to forcibly undo such commits, but it will be
good for the community to agree on the policy that deals with violations.

Ram, committing an improvement on top of a commit should be discouraged,
not encouraged as it eventually leads to the policy violation and lousy
PR
reviews.

Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:

I also thought that everybody was in agreement about that after the first

round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation,
I'm
more interested in a style of collaboration that does not require such
discussion.

Thomas

On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com
wrote:

Everybody seems agreed on what the committers should do -- that waiting
a


day or two for
others to have a chance to comment seems like an entirely reasonable
thing.

The disagreement is about what to do when that policy is violated.

Ram

On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:

Forced push should not be necessary if committers follow the
development


process.

So why not focus on what the committer should do? Development process
and
guidelines are there for a reason, and the issue was raised before.

In this specific case, there is a string of commits related to the
plugin
feature that IMO should be part of the original review. There wasn't
any
need to rush this, the change wasn't important for the release.

Thomas


On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
r...@datatorrent.com
wrote:

I agree with Pramod that force pushing should be a rare event done
only


when there is an immediate
need to undo something serious. Doing it just for a policy violation

should

itself be codified in our

policies as a policy violation.

Why not just commit an improvement on top ?

Ram

On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com
wrote:

Violation of the PR merge policy is a sufficient reason to forcibly
undo
the commit and such violations affect everyone in the community.


Thank you,

Vlad

On 4/27/17 19:36, Pramod Immaneni wrote:

I agree that PRs should not be merged without a chance for others to


review. I don't agree to force push and altering the commit tree

unless

it

is absolutely needed, as it affects everyone. This case doesn't

warrant


this step, one scenario where a force push might be needed is if


somebody
pushed some copyrighted code.


Thanks

On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <

v.ro...@datatorrent.com>

wrote:
I am open to both approaches - two commits or a join commit. Both

have

pros and cons that we may discuss. What I am strongly against are
PRs

that

are merged without a chance for other contributors/committers to

review.

There should be a way to forcibly undo such commits.


Thank you,

Vlad


On 4/27/17 12:41, Pramod Immaneni wrote:

My comments inline..

On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>

wrote:

I'm -1 on using the author field like this in Apex for the reason
stated
(it is also odd to see something like this showing up without
prior

discussion).

I am not set on this for future commits but would like to say,
do

we

really

verify the author field and treat it with importance. For

example, I

don't

think we ever check if the author is the person they say they are,

check

name, email etc. If I were to use slightly different variations of
my
name

or email (not that I would do that) would reviewers really verify

that.

I
also have checked that tools don't fail on reading a commit

because

author

needs to be in a certain form

Re: PR merge policy

I am sorry but I am -1 on the force push in this case.

> On Apr 27, 2017, at 9:27 PM, Thomas Weise <t...@apache.org> wrote:
>
> +1 as measure of last resort.
>
>> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:
>>
>> IMO, force push will bring enough consequent embarrassment to avoid such
>> behavior in the future.
>>
>> Thank you,
>>
>> Vlad
>>
>>> On 4/27/17 21:16, Munagala Ramanath wrote:
>>>
>>> My thought was that leaving the bad commit would be a permanent reminder
>>> to
>>> the committer
>>> (and others) that a policy violation occurred and the consequent
>>> embarrassment would be an
>>> adequate deterrent.
>>>
>>> Ram
>>>
>>> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>> wrote:
>>>
>>> I also was under impression that everyone agreed to the policy that gives
>>>> everyone in the community a chance to raise a concern or to propose an
>>>> improvement to a PR. Unfortunately, it is not the case, and we need to
>>>> discuss it again. I hope that this discussion will lead to no future
>>>> violations so we don't need to forcibly undo such commits, but it will be
>>>> good for the community to agree on the policy that deals with violations.
>>>>
>>>> Ram, committing an improvement on top of a commit should be discouraged,
>>>> not encouraged as it eventually leads to the policy violation and lousy
>>>> PR
>>>> reviews.
>>>>
>>>> Thank you,
>>>>
>>>> Vlad
>>>>
>>>> On 4/27/17 20:54, Thomas Weise wrote:
>>>>
>>>> I also thought that everybody was in agreement about that after the first
>>>>> round of discussion and as you say it would be hard to argue against it.
>>>>> And I think we should not have to be back to the same topic a few days
>>>>> later.
>>>>>
>>>>> While you seem to be focussed on the disagreement on policy violation,
>>>>> I'm
>>>>> more interested in a style of collaboration that does not require such
>>>>> discussion.
>>>>>
>>>>> Thomas
>>>>>
>>>>> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com
>>>>>>
>>>>> wrote:
>>>>>
>>>>> Everybody seems agreed on what the committers should do -- that waiting
>>>>> a
>>>>>
>>>>>> day or two for
>>>>>> others to have a chance to comment seems like an entirely reasonable
>>>>>> thing.
>>>>>>
>>>>>> The disagreement is about what to do when that policy is violated.
>>>>>>
>>>>>> Ram
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
>>>>>>
>>>>>> Forced push should not be necessary if committers follow the
>>>>>> development
>>>>>>
>>>>>>> process.
>>>>>>>
>>>>>>> So why not focus on what the committer should do? Development process
>>>>>>> and
>>>>>>> guidelines are there for a reason, and the issue was raised before.
>>>>>>>
>>>>>>> In this specific case, there is a string of commits related to the
>>>>>>> plugin
>>>>>>> feature that IMO should be part of the original review. There wasn't
>>>>>>> any
>>>>>>> need to rush this, the change wasn't important for the release.
>>>>>>>
>>>>>>> Thomas
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
>>>>>>> r...@datatorrent.com
>>>>>>> wrote:
>>>>>>>
>>>>>>> I agree with Pramod that force pushing should be a rare event done
>>>>>>> only
>>>>>>>
>>>>>>>> when there is an immediate
>>>>>>>> need to undo something serious. Doing it just for a policy violation
>>>>>>>>
>>>>>>>> should
>>>>>>>
>>>>>>> itself be codified in our
>>&g

Re: PR merge policy

Let's move forward and discuss ideas that will help decrease the
occurrence of this or prevent it and not do the force commit which is
disruptive and in my opinion not needed for this situation.

Thomas whatever your opinion of the review, it was not the cause of
this. Second, like I mentioned earlier this was not developed as a
single feature, the way things happened, to have been captured
everything in one PR or review and there were two features that were
later unified. Your point about not requiring it for the release is
well taken.

> On Apr 27, 2017, at 9:16 PM, Munagala Ramanath <r...@datatorrent.com> wrote:
>
> My thought was that leaving the bad commit would be a permanent reminder to
> the committer
> (and others) that a policy violation occurred and the consequent
> embarrassment would be an
> adequate deterrent.
>
> Ram
>
>> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:
>>
>> I also was under impression that everyone agreed to the policy that gives
>> everyone in the community a chance to raise a concern or to propose an
>> improvement to a PR. Unfortunately, it is not the case, and we need to
>> discuss it again. I hope that this discussion will lead to no future
>> violations so we don't need to forcibly undo such commits, but it will be
>> good for the community to agree on the policy that deals with violations.
>>
>> Ram, committing an improvement on top of a commit should be discouraged,
>> not encouraged as it eventually leads to the policy violation and lousy PR
>> reviews.
>>
>> Thank you,
>>
>> Vlad
>>
>>> On 4/27/17 20:54, Thomas Weise wrote:
>>>
>>> I also thought that everybody was in agreement about that after the first
>>> round of discussion and as you say it would be hard to argue against it.
>>> And I think we should not have to be back to the same topic a few days
>>> later.
>>>
>>> While you seem to be focussed on the disagreement on policy violation, I'm
>>> more interested in a style of collaboration that does not require such
>>> discussion.
>>>
>>> Thomas
>>>
>>> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
>>> wrote:
>>>
>>> Everybody seems agreed on what the committers should do -- that waiting a
>>>> day or two for
>>>> others to have a chance to comment seems like an entirely reasonable
>>>> thing.
>>>>
>>>> The disagreement is about what to do when that policy is violated.
>>>>
>>>> Ram
>>>>
>>>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
>>>>
>>>> Forced push should not be necessary if committers follow the development
>>>>> process.
>>>>>
>>>>> So why not focus on what the committer should do? Development process
>>>>> and
>>>>> guidelines are there for a reason, and the issue was raised before.
>>>>>
>>>>> In this specific case, there is a string of commits related to the
>>>>> plugin
>>>>> feature that IMO should be part of the original review. There wasn't any
>>>>> need to rush this, the change wasn't important for the release.
>>>>>
>>>>> Thomas
>>>>>
>>>>>
>>>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com
>>>>>>
>>>>> wrote:
>>>>>
>>>>> I agree with Pramod that force pushing should be a rare event done only
>>>>>> when there is an immediate
>>>>>> need to undo something serious. Doing it just for a policy violation
>>>>>>
>>>>> should
>>>>>
>>>>>> itself be codified in our
>>>>>> policies as a policy violation.
>>>>>>
>>>>>> Why not just commit an improvement on top ?
>>>>>>
>>>>>> Ram
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>>>>> wrote:
>>>>>>
>>>>>> Violation of the PR merge policy is a sufficient reason to forcibly
>>>>>>>
>>>>>> undo
>>>>>
>>>>>> the commit and such violations affect everyone in the community.
>>>>>>>
>>>>>>> Thank you,
>>>>>>>
>>>>&g

Re: PR merge policy

+1 as measure of last resort.

On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> IMO, force push will bring enough consequent embarrassment to avoid such
> behavior in the future.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 21:16, Munagala Ramanath wrote:
>
>> My thought was that leaving the bad commit would be a permanent reminder
>> to
>> the committer
>> (and others) that a policy violation occurred and the consequent
>> embarrassment would be an
>> adequate deterrent.
>>
>> Ram
>>
>> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
>> wrote:
>>
>> I also was under impression that everyone agreed to the policy that gives
>>> everyone in the community a chance to raise a concern or to propose an
>>> improvement to a PR. Unfortunately, it is not the case, and we need to
>>> discuss it again. I hope that this discussion will lead to no future
>>> violations so we don't need to forcibly undo such commits, but it will be
>>> good for the community to agree on the policy that deals with violations.
>>>
>>> Ram, committing an improvement on top of a commit should be discouraged,
>>> not encouraged as it eventually leads to the policy violation and lousy
>>> PR
>>> reviews.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>> On 4/27/17 20:54, Thomas Weise wrote:
>>>
>>> I also thought that everybody was in agreement about that after the first
>>>> round of discussion and as you say it would be hard to argue against it.
>>>> And I think we should not have to be back to the same topic a few days
>>>> later.
>>>>
>>>> While you seem to be focussed on the disagreement on policy violation,
>>>> I'm
>>>> more interested in a style of collaboration that does not require such
>>>> discussion.
>>>>
>>>> Thomas
>>>>
>>>> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com
>>>> >
>>>> wrote:
>>>>
>>>> Everybody seems agreed on what the committers should do -- that waiting
>>>> a
>>>>
>>>>> day or two for
>>>>> others to have a chance to comment seems like an entirely reasonable
>>>>> thing.
>>>>>
>>>>> The disagreement is about what to do when that policy is violated.
>>>>>
>>>>> Ram
>>>>>
>>>>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
>>>>>
>>>>> Forced push should not be necessary if committers follow the
>>>>> development
>>>>>
>>>>>> process.
>>>>>>
>>>>>> So why not focus on what the committer should do? Development process
>>>>>> and
>>>>>> guidelines are there for a reason, and the issue was raised before.
>>>>>>
>>>>>> In this specific case, there is a string of commits related to the
>>>>>> plugin
>>>>>> feature that IMO should be part of the original review. There wasn't
>>>>>> any
>>>>>> need to rush this, the change wasn't important for the release.
>>>>>>
>>>>>> Thomas
>>>>>>
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
>>>>>> r...@datatorrent.com
>>>>>> wrote:
>>>>>>
>>>>>> I agree with Pramod that force pushing should be a rare event done
>>>>>> only
>>>>>>
>>>>>>> when there is an immediate
>>>>>>> need to undo something serious. Doing it just for a policy violation
>>>>>>>
>>>>>>> should
>>>>>>
>>>>>> itself be codified in our
>>>>>>> policies as a policy violation.
>>>>>>>
>>>>>>> Why not just commit an improvement on top ?
>>>>>>>
>>>>>>> Ram
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com
>>>>>>> >
>>>>>>> wrote:
>>>>>>>
>>>>>>> Violation of the PR merge policy is a sufficient reason to forcibly
>>>>>>> undo

Re: PR merge policy

That's an interesting thought also. Here is one more: Committer on the
project should require demonstrated responsible behavior in this area.
Proven ability to collaborate and doing the right things will be a criteria
for such nominations on my list.


On Thu, Apr 27, 2017 at 9:16 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:

> My thought was that leaving the bad commit would be a permanent reminder to
> the committer
> (and others) that a policy violation occurred and the consequent
> embarrassment would be an
> adequate deterrent.
>
> Ram
>
> On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> > I also was under impression that everyone agreed to the policy that gives
> > everyone in the community a chance to raise a concern or to propose an
> > improvement to a PR. Unfortunately, it is not the case, and we need to
> > discuss it again. I hope that this discussion will lead to no future
> > violations so we don't need to forcibly undo such commits, but it will be
> > good for the community to agree on the policy that deals with violations.
> >
> > Ram, committing an improvement on top of a commit should be discouraged,
> > not encouraged as it eventually leads to the policy violation and lousy
> PR
> > reviews.
> >
> > Thank you,
> >
> > Vlad
> >
> > On 4/27/17 20:54, Thomas Weise wrote:
> >
> >> I also thought that everybody was in agreement about that after the
> first
> >> round of discussion and as you say it would be hard to argue against it.
> >> And I think we should not have to be back to the same topic a few days
> >> later.
> >>
> >> While you seem to be focussed on the disagreement on policy violation,
> I'm
> >> more interested in a style of collaboration that does not require such
> >> discussion.
> >>
> >> Thomas
> >>
> >> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com
> >
> >> wrote:
> >>
> >> Everybody seems agreed on what the committers should do -- that waiting
> a
> >>> day or two for
> >>> others to have a chance to comment seems like an entirely reasonable
> >>> thing.
> >>>
> >>> The disagreement is about what to do when that policy is violated.
> >>>
> >>> Ram
> >>>
> >>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
> >>>
> >>> Forced push should not be necessary if committers follow the
> development
> >>>> process.
> >>>>
> >>>> So why not focus on what the committer should do? Development process
> >>>> and
> >>>> guidelines are there for a reason, and the issue was raised before.
> >>>>
> >>>> In this specific case, there is a string of commits related to the
> >>>> plugin
> >>>> feature that IMO should be part of the original review. There wasn't
> any
> >>>> need to rush this, the change wasn't important for the release.
> >>>>
> >>>> Thomas
> >>>>
> >>>>
> >>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
> r...@datatorrent.com
> >>>> >
> >>>> wrote:
> >>>>
> >>>> I agree with Pramod that force pushing should be a rare event done
> only
> >>>>> when there is an immediate
> >>>>> need to undo something serious. Doing it just for a policy violation
> >>>>>
> >>>> should
> >>>>
> >>>>> itself be codified in our
> >>>>> policies as a policy violation.
> >>>>>
> >>>>> Why not just commit an improvement on top ?
> >>>>>
> >>>>> Ram
> >>>>>
> >>>>> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com
> >
> >>>>> wrote:
> >>>>>
> >>>>> Violation of the PR merge policy is a sufficient reason to forcibly
> >>>>>>
> >>>>> undo
> >>>>
> >>>>> the commit and such violations affect everyone in the community.
> >>>>>>
> >>>>>> Thank you,
> >>>>>>
> >>>>>> Vlad
> >>>>>>
> >>>>>> On 4/27/17 19:36, Pramod Immaneni wrote:
> >>>>>>
> >>>&g

Re: PR merge policy

IMO, force push will bring enough consequent embarrassment to avoid such 
behavior in the future.


Thank you,

Vlad
On 4/27/17 21:16, Munagala Ramanath wrote:

My thought was that leaving the bad commit would be a permanent reminder to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


I also was under impression that everyone agreed to the policy that gives
everyone in the community a chance to raise a concern or to propose an
improvement to a PR. Unfortunately, it is not the case, and we need to
discuss it again. I hope that this discussion will lead to no future
violations so we don't need to forcibly undo such commits, but it will be
good for the community to agree on the policy that deals with violations.

Ram, committing an improvement on top of a commit should be discouraged,
not encouraged as it eventually leads to the policy violation and lousy PR
reviews.

Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:


I also thought that everybody was in agreement about that after the first
round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation, I'm
more interested in a style of collaboration that does not require such
discussion.

Thomas

On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:

Everybody seems agreed on what the committers should do -- that waiting a

day or two for
others to have a chance to comment seems like an entirely reasonable
thing.

The disagreement is about what to do when that policy is violated.

Ram

On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:

Forced push should not be necessary if committers follow the development

process.

So why not focus on what the committer should do? Development process
and
guidelines are there for a reason, and the issue was raised before.

In this specific case, there is a string of commits related to the
plugin
feature that IMO should be part of the original review. There wasn't any
need to rush this, the change wasn't important for the release.

Thomas


On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com
wrote:

I agree with Pramod that force pushing should be a rare event done only

when there is an immediate
need to undo something serious. Doing it just for a policy violation


should


itself be codified in our
policies as a policy violation.

Why not just commit an improvement on top ?

Ram

On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

Violation of the PR merge policy is a sufficient reason to forcibly
undo
the commit and such violations affect everyone in the community.

Thank you,

Vlad

On 4/27/17 19:36, Pramod Immaneni wrote:

I agree that PRs should not be merged without a chance for others to

review. I don't agree to force push and altering the commit tree


unless

it


is absolutely needed, as it affects everyone. This case doesn't
warrant

this step, one scenario where a force push might be needed is if

somebody
pushed some copyrighted code.

Thanks

On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <


v.ro...@datatorrent.com>

wrote:

I am open to both approaches - two commits or a join commit. Both


have

pros and cons that we may discuss. What I am strongly against are

PRs

that

are merged without a chance for other contributors/committers to


review.

There should be a way to forcibly undo such commits.

Thank you,

Vlad


On 4/27/17 12:41, Pramod Immaneni wrote:

My comments inline..


On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>


wrote:

I'm -1 on using the author field like this in Apex for the reason

stated

(it is also odd to see something like this showing up without

prior

discussion).


I am not set on this for future commits but would like to say, do


we

really

verify the author field and treat it with importance. For


example, I

don't

think we ever check if the author is the person they say they are,


check

name, email etc. If I were to use slightly different variations of

my

name

or email (not that I would do that) would reviewers really verify


that.

I

also have checked that tools don't fail on reading a commit


because

author

needs to be in a certain format. I guess contribution stats are


the

ones

that will be affected but if used rarely I dont see that being a

big

problem. I can understand if we wanted to have strict requirements

for

the

committer field.

Thanks


Consider adding the additional author information to the commit


message.

Thomas

On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
pra...@datatorrent.com>
wrote:

Agreed it is not regular and should only be used in specia

Re: PR merge policy

My thought was that leaving the bad commit would be a permanent reminder to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> I also was under impression that everyone agreed to the policy that gives
> everyone in the community a chance to raise a concern or to propose an
> improvement to a PR. Unfortunately, it is not the case, and we need to
> discuss it again. I hope that this discussion will lead to no future
> violations so we don't need to forcibly undo such commits, but it will be
> good for the community to agree on the policy that deals with violations.
>
> Ram, committing an improvement on top of a commit should be discouraged,
> not encouraged as it eventually leads to the policy violation and lousy PR
> reviews.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 20:54, Thomas Weise wrote:
>
>> I also thought that everybody was in agreement about that after the first
>> round of discussion and as you say it would be hard to argue against it.
>> And I think we should not have to be back to the same topic a few days
>> later.
>>
>> While you seem to be focussed on the disagreement on policy violation, I'm
>> more interested in a style of collaboration that does not require such
>> discussion.
>>
>> Thomas
>>
>> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
>> wrote:
>>
>> Everybody seems agreed on what the committers should do -- that waiting a
>>> day or two for
>>> others to have a chance to comment seems like an entirely reasonable
>>> thing.
>>>
>>> The disagreement is about what to do when that policy is violated.
>>>
>>> Ram
>>>
>>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
>>>
>>> Forced push should not be necessary if committers follow the development
>>>> process.
>>>>
>>>> So why not focus on what the committer should do? Development process
>>>> and
>>>> guidelines are there for a reason, and the issue was raised before.
>>>>
>>>> In this specific case, there is a string of commits related to the
>>>> plugin
>>>> feature that IMO should be part of the original review. There wasn't any
>>>> need to rush this, the change wasn't important for the release.
>>>>
>>>> Thomas
>>>>
>>>>
>>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com
>>>> >
>>>> wrote:
>>>>
>>>> I agree with Pramod that force pushing should be a rare event done only
>>>>> when there is an immediate
>>>>> need to undo something serious. Doing it just for a policy violation
>>>>>
>>>> should
>>>>
>>>>> itself be codified in our
>>>>> policies as a policy violation.
>>>>>
>>>>> Why not just commit an improvement on top ?
>>>>>
>>>>> Ram
>>>>>
>>>>> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
>>>>> wrote:
>>>>>
>>>>> Violation of the PR merge policy is a sufficient reason to forcibly
>>>>>>
>>>>> undo
>>>>
>>>>> the commit and such violations affect everyone in the community.
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Vlad
>>>>>>
>>>>>> On 4/27/17 19:36, Pramod Immaneni wrote:
>>>>>>
>>>>>> I agree that PRs should not be merged without a chance for others to
>>>>>>> review. I don't agree to force push and altering the commit tree
>>>>>>>
>>>>>> unless
>>>>
>>>>> it
>>>>>
>>>>>> is absolutely needed, as it affects everyone. This case doesn't
>>>>>>>
>>>>>> warrant
>>>>
>>>>> this step, one scenario where a force push might be needed is if
>>>>>>>
>>>>>> somebody
>>>>>
>>>>>> pushed some copyrighted code.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <
>>>>>>>
>&

Re: PR merge policy

Haha. Community won't grow without idealism.


On Thu, Apr 27, 2017 at 9:03 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:

> Idealism is a wonderful thing but reality sometimes intrudes.
>
> Ram
>
> On Thu, Apr 27, 2017 at 8:54 PM, Thomas Weise <t...@apache.org> wrote:
>
> > I also thought that everybody was in agreement about that after the first
> > round of discussion and as you say it would be hard to argue against it.
> > And I think we should not have to be back to the same topic a few days
> > later.
> >
> > While you seem to be focussed on the disagreement on policy violation,
> I'm
> > more interested in a style of collaboration that does not require such
> > discussion.
> >
> > Thomas
> >
> > On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
> > wrote:
> >
> > > Everybody seems agreed on what the committers should do -- that
> waiting a
> > > day or two for
> > > others to have a chance to comment seems like an entirely reasonable
> > thing.
> > >
> > > The disagreement is about what to do when that policy is violated.
> > >
> > > Ram
> > >
> > > On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
> > >
> > > > Forced push should not be necessary if committers follow the
> > development
> > > > process.
> > > >
> > > > So why not focus on what the committer should do? Development process
> > and
> > > > guidelines are there for a reason, and the issue was raised before.
> > > >
> > > > In this specific case, there is a string of commits related to the
> > plugin
> > > > feature that IMO should be part of the original review. There wasn't
> > any
> > > > need to rush this, the change wasn't important for the release.
> > > >
> > > > Thomas
> > > >
> > > >
> > > > On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
> > r...@datatorrent.com>
> > > > wrote:
> > > >
> > > > > I agree with Pramod that force pushing should be a rare event done
> > only
> > > > > when there is an immediate
> > > > > need to undo something serious. Doing it just for a policy
> violation
> > > > should
> > > > > itself be codified in our
> > > > > policies as a policy violation.
> > > > >
> > > > > Why not just commit an improvement on top ?
> > > > >
> > > > > Ram
> > > > >
> > > > > On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <
> v.ro...@datatorrent.com
> > >
> > > > > wrote:
> > > > >
> > > > > > Violation of the PR merge policy is a sufficient reason to
> forcibly
> > > > undo
> > > > > > the commit and such violations affect everyone in the community.
> > > > > >
> > > > > > Thank you,
> > > > > >
> > > > > > Vlad
> > > > > >
> > > > > > On 4/27/17 19:36, Pramod Immaneni wrote:
> > > > > >
> > > > > >> I agree that PRs should not be merged without a chance for
> others
> > to
> > > > > >> review. I don't agree to force push and altering the commit tree
> > > > unless
> > > > > it
> > > > > >> is absolutely needed, as it affects everyone. This case doesn't
> > > > warrant
> > > > > >> this step, one scenario where a force push might be needed is if
> > > > > somebody
> > > > > >> pushed some copyrighted code.
> > > > > >>
> > > > > >> Thanks
> > > > > >>
> > > > > >> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <
> > > v.ro...@datatorrent.com>
> > > > > >> wrote:
> > > > > >>
> > > > > >> I am open to both approaches - two commits or a join commit.
> Both
> > > have
> > > > > >>> pros and cons that we may discuss. What I am strongly against
> are
> > > PRs
> > > > > >>> that
> > > > > >>> are merged without a chance for other contributors/committers
> to
> > > > > review.
> > > > > >>> There should be a way to forcibly undo such commits.
> > > > >

Re: PR merge policy

I also was under impression that everyone agreed to the policy that 
gives everyone in the community a chance to raise a concern or to 
propose an improvement to a PR. Unfortunately, it is not the case, and 
we need to discuss it again. I hope that this discussion will lead to no 
future violations so we don't need to forcibly undo such commits, but it 
will be good for the community to agree on the policy that deals with 
violations.


Ram, committing an improvement on top of a commit should be discouraged, 
not encouraged as it eventually leads to the policy violation and lousy 
PR reviews.


Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:

I also thought that everybody was in agreement about that after the first
round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation, I'm
more interested in a style of collaboration that does not require such
discussion.

Thomas

On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:


Everybody seems agreed on what the committers should do -- that waiting a
day or two for
others to have a chance to comment seems like an entirely reasonable thing.

The disagreement is about what to do when that policy is violated.

Ram

On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:


Forced push should not be necessary if committers follow the development
process.

So why not focus on what the committer should do? Development process and
guidelines are there for a reason, and the issue was raised before.

In this specific case, there is a string of commits related to the plugin
feature that IMO should be part of the original review. There wasn't any
need to rush this, the change wasn't important for the release.

Thomas


On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:


I agree with Pramod that force pushing should be a rare event done only
when there is an immediate
need to undo something serious. Doing it just for a policy violation

should

itself be codified in our
policies as a policy violation.

Why not just commit an improvement on top ?

Ram

On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:


Violation of the PR merge policy is a sufficient reason to forcibly

undo

the commit and such violations affect everyone in the community.

Thank you,

Vlad

On 4/27/17 19:36, Pramod Immaneni wrote:


I agree that PRs should not be merged without a chance for others to
review. I don't agree to force push and altering the commit tree

unless

it

is absolutely needed, as it affects everyone. This case doesn't

warrant

this step, one scenario where a force push might be needed is if

somebody

pushed some copyrighted code.

Thanks

On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <

v.ro...@datatorrent.com>

wrote:

I am open to both approaches - two commits or a join commit. Both

have

pros and cons that we may discuss. What I am strongly against are

PRs

that
are merged without a chance for other contributors/committers to

review.

There should be a way to forcibly undo such commits.

Thank you,

Vlad


On 4/27/17 12:41, Pramod Immaneni wrote:

My comments inline..

On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>

wrote:

I'm -1 on using the author field like this in Apex for the reason

stated

(it is also odd to see something like this showing up without

prior

discussion).


I am not set on this for future commits but would like to say, do

we

really
verify the author field and treat it with importance. For

example, I

don't
think we ever check if the author is the person they say they are,

check

name, email etc. If I were to use slightly different variations of

my

name
or email (not that I would do that) would reviewers really verify

that.

I
also have checked that tools don't fail on reading a commit

because

author
needs to be in a certain format. I guess contribution stats are

the

ones

that will be affected but if used rarely I dont see that being a

big

problem. I can understand if we wanted to have strict requirements

for

the
committer field.

Thanks


Consider adding the additional author information to the commit

message.

Thomas

On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
pra...@datatorrent.com>
wrote:

Agreed it is not regular and should only be used in special
circumstances.

One example of this is pair programming. It has been done before

in

other
projects and searching on google or stackoverflow you can see

how

other
people have tried to handle it

https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
http://stackoverflow.com/questions/7442112/attributing-
a-single-commit-to-multiple-developers

Thanks

On Thu, Apr 27, 2017 at 9:57 AM, Thom

Re: PR merge policy

Idealism is a wonderful thing but reality sometimes intrudes.

Ram

On Thu, Apr 27, 2017 at 8:54 PM, Thomas Weise <t...@apache.org> wrote:

> I also thought that everybody was in agreement about that after the first
> round of discussion and as you say it would be hard to argue against it.
> And I think we should not have to be back to the same topic a few days
> later.
>
> While you seem to be focussed on the disagreement on policy violation, I'm
> more interested in a style of collaboration that does not require such
> discussion.
>
> Thomas
>
> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
> wrote:
>
> > Everybody seems agreed on what the committers should do -- that waiting a
> > day or two for
> > others to have a chance to comment seems like an entirely reasonable
> thing.
> >
> > The disagreement is about what to do when that policy is violated.
> >
> > Ram
> >
> > On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
> >
> > > Forced push should not be necessary if committers follow the
> development
> > > process.
> > >
> > > So why not focus on what the committer should do? Development process
> and
> > > guidelines are there for a reason, and the issue was raised before.
> > >
> > > In this specific case, there is a string of commits related to the
> plugin
> > > feature that IMO should be part of the original review. There wasn't
> any
> > > need to rush this, the change wasn't important for the release.
> > >
> > > Thomas
> > >
> > >
> > > On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <
> r...@datatorrent.com>
> > > wrote:
> > >
> > > > I agree with Pramod that force pushing should be a rare event done
> only
> > > > when there is an immediate
> > > > need to undo something serious. Doing it just for a policy violation
> > > should
> > > > itself be codified in our
> > > > policies as a policy violation.
> > > >
> > > > Why not just commit an improvement on top ?
> > > >
> > > > Ram
> > > >
> > > > On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com
> >
> > > > wrote:
> > > >
> > > > > Violation of the PR merge policy is a sufficient reason to forcibly
> > > undo
> > > > > the commit and such violations affect everyone in the community.
> > > > >
> > > > > Thank you,
> > > > >
> > > > > Vlad
> > > > >
> > > > > On 4/27/17 19:36, Pramod Immaneni wrote:
> > > > >
> > > > >> I agree that PRs should not be merged without a chance for others
> to
> > > > >> review. I don't agree to force push and altering the commit tree
> > > unless
> > > > it
> > > > >> is absolutely needed, as it affects everyone. This case doesn't
> > > warrant
> > > > >> this step, one scenario where a force push might be needed is if
> > > > somebody
> > > > >> pushed some copyrighted code.
> > > > >>
> > > > >> Thanks
> > > > >>
> > > > >> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <
> > v.ro...@datatorrent.com>
> > > > >> wrote:
> > > > >>
> > > > >> I am open to both approaches - two commits or a join commit. Both
> > have
> > > > >>> pros and cons that we may discuss. What I am strongly against are
> > PRs
> > > > >>> that
> > > > >>> are merged without a chance for other contributors/committers to
> > > > review.
> > > > >>> There should be a way to forcibly undo such commits.
> > > > >>>
> > > > >>> Thank you,
> > > > >>>
> > > > >>> Vlad
> > > > >>>
> > > > >>>
> > > > >>> On 4/27/17 12:41, Pramod Immaneni wrote:
> > > > >>>
> > > > >>> My comments inline..
> > > > >>>>
> > > > >>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>
> > > > wrote:
> > > > >>>>
> > > > >>>> I'm -1 on using the author field like this in Apex for the
> reas

Re: PR merge policy

I also thought that everybody was in agreement about that after the first
round of discussion and as you say it would be hard to argue against it.
And I think we should not have to be back to the same topic a few days
later.

While you seem to be focussed on the disagreement on policy violation, I'm
more interested in a style of collaboration that does not require such
discussion.

Thomas

On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:

> Everybody seems agreed on what the committers should do -- that waiting a
> day or two for
> others to have a chance to comment seems like an entirely reasonable thing.
>
> The disagreement is about what to do when that policy is violated.
>
> Ram
>
> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:
>
> > Forced push should not be necessary if committers follow the development
> > process.
> >
> > So why not focus on what the committer should do? Development process and
> > guidelines are there for a reason, and the issue was raised before.
> >
> > In this specific case, there is a string of commits related to the plugin
> > feature that IMO should be part of the original review. There wasn't any
> > need to rush this, the change wasn't important for the release.
> >
> > Thomas
> >
> >
> > On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com>
> > wrote:
> >
> > > I agree with Pramod that force pushing should be a rare event done only
> > > when there is an immediate
> > > need to undo something serious. Doing it just for a policy violation
> > should
> > > itself be codified in our
> > > policies as a policy violation.
> > >
> > > Why not just commit an improvement on top ?
> > >
> > > Ram
> > >
> > > On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
> > > wrote:
> > >
> > > > Violation of the PR merge policy is a sufficient reason to forcibly
> > undo
> > > > the commit and such violations affect everyone in the community.
> > > >
> > > > Thank you,
> > > >
> > > > Vlad
> > > >
> > > > On 4/27/17 19:36, Pramod Immaneni wrote:
> > > >
> > > >> I agree that PRs should not be merged without a chance for others to
> > > >> review. I don't agree to force push and altering the commit tree
> > unless
> > > it
> > > >> is absolutely needed, as it affects everyone. This case doesn't
> > warrant
> > > >> this step, one scenario where a force push might be needed is if
> > > somebody
> > > >> pushed some copyrighted code.
> > > >>
> > > >> Thanks
> > > >>
> > > >> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <
> v.ro...@datatorrent.com>
> > > >> wrote:
> > > >>
> > > >> I am open to both approaches - two commits or a join commit. Both
> have
> > > >>> pros and cons that we may discuss. What I am strongly against are
> PRs
> > > >>> that
> > > >>> are merged without a chance for other contributors/committers to
> > > review.
> > > >>> There should be a way to forcibly undo such commits.
> > > >>>
> > > >>> Thank you,
> > > >>>
> > > >>> Vlad
> > > >>>
> > > >>>
> > > >>> On 4/27/17 12:41, Pramod Immaneni wrote:
> > > >>>
> > > >>> My comments inline..
> > > >>>>
> > > >>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>
> > > wrote:
> > > >>>>
> > > >>>> I'm -1 on using the author field like this in Apex for the reason
> > > stated
> > > >>>>
> > > >>>>> (it is also odd to see something like this showing up without
> prior
> > > >>>>> discussion).
> > > >>>>>
> > > >>>>>
> > > >>>>> I am not set on this for future commits but would like to say, do
> > we
> > > >>>>>
> > > >>>> really
> > > >>>> verify the author field and treat it with importance. For
> example, I
> > > >>>> don't
> > > >>>> think we ever check if the author is the person they say they are,

Re: PR merge policy

Everybody seems agreed on what the committers should do -- that waiting a
day or two for
others to have a chance to comment seems like an entirely reasonable thing.

The disagreement is about what to do when that policy is violated.

Ram

On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote:

> Forced push should not be necessary if committers follow the development
> process.
>
> So why not focus on what the committer should do? Development process and
> guidelines are there for a reason, and the issue was raised before.
>
> In this specific case, there is a string of commits related to the plugin
> feature that IMO should be part of the original review. There wasn't any
> need to rush this, the change wasn't important for the release.
>
> Thomas
>
>
> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com>
> wrote:
>
> > I agree with Pramod that force pushing should be a rare event done only
> > when there is an immediate
> > need to undo something serious. Doing it just for a policy violation
> should
> > itself be codified in our
> > policies as a policy violation.
> >
> > Why not just commit an improvement on top ?
> >
> > Ram
> >
> > On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
> > wrote:
> >
> > > Violation of the PR merge policy is a sufficient reason to forcibly
> undo
> > > the commit and such violations affect everyone in the community.
> > >
> > > Thank you,
> > >
> > > Vlad
> > >
> > > On 4/27/17 19:36, Pramod Immaneni wrote:
> > >
> > >> I agree that PRs should not be merged without a chance for others to
> > >> review. I don't agree to force push and altering the commit tree
> unless
> > it
> > >> is absolutely needed, as it affects everyone. This case doesn't
> warrant
> > >> this step, one scenario where a force push might be needed is if
> > somebody
> > >> pushed some copyrighted code.
> > >>
> > >> Thanks
> > >>
> > >> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <v.ro...@datatorrent.com>
> > >> wrote:
> > >>
> > >> I am open to both approaches - two commits or a join commit. Both have
> > >>> pros and cons that we may discuss. What I am strongly against are PRs
> > >>> that
> > >>> are merged without a chance for other contributors/committers to
> > review.
> > >>> There should be a way to forcibly undo such commits.
> > >>>
> > >>> Thank you,
> > >>>
> > >>> Vlad
> > >>>
> > >>>
> > >>> On 4/27/17 12:41, Pramod Immaneni wrote:
> > >>>
> > >>> My comments inline..
> > >>>>
> > >>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>
> > wrote:
> > >>>>
> > >>>> I'm -1 on using the author field like this in Apex for the reason
> > stated
> > >>>>
> > >>>>> (it is also odd to see something like this showing up without prior
> > >>>>> discussion).
> > >>>>>
> > >>>>>
> > >>>>> I am not set on this for future commits but would like to say, do
> we
> > >>>>>
> > >>>> really
> > >>>> verify the author field and treat it with importance. For example, I
> > >>>> don't
> > >>>> think we ever check if the author is the person they say they are,
> > check
> > >>>> name, email etc. If I were to use slightly different variations of
> my
> > >>>> name
> > >>>> or email (not that I would do that) would reviewers really verify
> > that.
> > >>>> I
> > >>>> also have checked that tools don't fail on reading a commit because
> > >>>> author
> > >>>> needs to be in a certain format. I guess contribution stats are the
> > ones
> > >>>> that will be affected but if used rarely I dont see that being a big
> > >>>> problem. I can understand if we wanted to have strict requirements
> for
> > >>>> the
> > >>>> committer field.
> > >>>>
> > >>>> Thanks
> > >>>>
> > >>>>
> > >>>> Consider adding the additional author information

Re: PR merge policy

Forced push should not be necessary if committers follow the development
process.

So why not focus on what the committer should do? Development process and
guidelines are there for a reason, and the issue was raised before.

In this specific case, there is a string of commits related to the plugin
feature that IMO should be part of the original review. There wasn't any
need to rush this, the change wasn't important for the release.

Thomas


On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com>
wrote:

> I agree with Pramod that force pushing should be a rare event done only
> when there is an immediate
> need to undo something serious. Doing it just for a policy violation should
> itself be codified in our
> policies as a policy violation.
>
> Why not just commit an improvement on top ?
>
> Ram
>
> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com>
> wrote:
>
> > Violation of the PR merge policy is a sufficient reason to forcibly undo
> > the commit and such violations affect everyone in the community.
> >
> > Thank you,
> >
> > Vlad
> >
> > On 4/27/17 19:36, Pramod Immaneni wrote:
> >
> >> I agree that PRs should not be merged without a chance for others to
> >> review. I don't agree to force push and altering the commit tree unless
> it
> >> is absolutely needed, as it affects everyone. This case doesn't warrant
> >> this step, one scenario where a force push might be needed is if
> somebody
> >> pushed some copyrighted code.
> >>
> >> Thanks
> >>
> >> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <v.ro...@datatorrent.com>
> >> wrote:
> >>
> >> I am open to both approaches - two commits or a join commit. Both have
> >>> pros and cons that we may discuss. What I am strongly against are PRs
> >>> that
> >>> are merged without a chance for other contributors/committers to
> review.
> >>> There should be a way to forcibly undo such commits.
> >>>
> >>> Thank you,
> >>>
> >>> Vlad
> >>>
> >>>
> >>> On 4/27/17 12:41, Pramod Immaneni wrote:
> >>>
> >>> My comments inline..
> >>>>
> >>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org>
> wrote:
> >>>>
> >>>> I'm -1 on using the author field like this in Apex for the reason
> stated
> >>>>
> >>>>> (it is also odd to see something like this showing up without prior
> >>>>> discussion).
> >>>>>
> >>>>>
> >>>>> I am not set on this for future commits but would like to say, do we
> >>>>>
> >>>> really
> >>>> verify the author field and treat it with importance. For example, I
> >>>> don't
> >>>> think we ever check if the author is the person they say they are,
> check
> >>>> name, email etc. If I were to use slightly different variations of my
> >>>> name
> >>>> or email (not that I would do that) would reviewers really verify
> that.
> >>>> I
> >>>> also have checked that tools don't fail on reading a commit because
> >>>> author
> >>>> needs to be in a certain format. I guess contribution stats are the
> ones
> >>>> that will be affected but if used rarely I dont see that being a big
> >>>> problem. I can understand if we wanted to have strict requirements for
> >>>> the
> >>>> committer field.
> >>>>
> >>>> Thanks
> >>>>
> >>>>
> >>>> Consider adding the additional author information to the commit
> message.
> >>>>
> >>>>> Thomas
> >>>>>
> >>>>> On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
> >>>>> pra...@datatorrent.com>
> >>>>> wrote:
> >>>>>
> >>>>> Agreed it is not regular and should only be used in special
> >>>>> circumstances.
> >>>>>
> >>>>> One example of this is pair programming. It has been done before in
> >>>>>> other
> >>>>>> projects and searching on google or stackoverflow you can see how
> >>>>>> other
> >>>>>> people have tried to handle it
> >>>>>>
> >>>>>> https://bugs.eclipse.org/bugs/

Re: PR merge policy

I agree with Pramod that force pushing should be a rare event done only
when there is an immediate
need to undo something serious. Doing it just for a policy violation should
itself be codified in our
policies as a policy violation.

Why not just commit an improvement on top ?

Ram

On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> Violation of the PR merge policy is a sufficient reason to forcibly undo
> the commit and such violations affect everyone in the community.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 19:36, Pramod Immaneni wrote:
>
>> I agree that PRs should not be merged without a chance for others to
>> review. I don't agree to force push and altering the commit tree unless it
>> is absolutely needed, as it affects everyone. This case doesn't warrant
>> this step, one scenario where a force push might be needed is if somebody
>> pushed some copyrighted code.
>>
>> Thanks
>>
>> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <v.ro...@datatorrent.com>
>> wrote:
>>
>> I am open to both approaches - two commits or a join commit. Both have
>>> pros and cons that we may discuss. What I am strongly against are PRs
>>> that
>>> are merged without a chance for other contributors/committers to review.
>>> There should be a way to forcibly undo such commits.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>>
>>> On 4/27/17 12:41, Pramod Immaneni wrote:
>>>
>>> My comments inline..
>>>>
>>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org> wrote:
>>>>
>>>> I'm -1 on using the author field like this in Apex for the reason stated
>>>>
>>>>> (it is also odd to see something like this showing up without prior
>>>>> discussion).
>>>>>
>>>>>
>>>>> I am not set on this for future commits but would like to say, do we
>>>>>
>>>> really
>>>> verify the author field and treat it with importance. For example, I
>>>> don't
>>>> think we ever check if the author is the person they say they are, check
>>>> name, email etc. If I were to use slightly different variations of my
>>>> name
>>>> or email (not that I would do that) would reviewers really verify that.
>>>> I
>>>> also have checked that tools don't fail on reading a commit because
>>>> author
>>>> needs to be in a certain format. I guess contribution stats are the ones
>>>> that will be affected but if used rarely I dont see that being a big
>>>> problem. I can understand if we wanted to have strict requirements for
>>>> the
>>>> committer field.
>>>>
>>>> Thanks
>>>>
>>>>
>>>> Consider adding the additional author information to the commit message.
>>>>
>>>>> Thomas
>>>>>
>>>>> On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
>>>>> pra...@datatorrent.com>
>>>>> wrote:
>>>>>
>>>>> Agreed it is not regular and should only be used in special
>>>>> circumstances.
>>>>>
>>>>> One example of this is pair programming. It has been done before in
>>>>>> other
>>>>>> projects and searching on google or stackoverflow you can see how
>>>>>> other
>>>>>> people have tried to handle it
>>>>>>
>>>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
>>>>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
>>>>>> http://stackoverflow.com/questions/7442112/attributing-
>>>>>> a-single-commit-to-multiple-developers
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> wrote:
>>>>>>
>>>>>> commit 9856080ede62a4529d730bcb6724c757f5010990
>>>>>>
>>>>>>> Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com
>>>>>>> >
>>>>>>> Date:   Tue Apr 18 09:37:22 2017 -0700
>>>>>>>
>>>>>>> Please don't use the author field in such a way, it leads to
>>>>>>> incorrect
>>>>>>> tracking of contributions.
>>>>>>>
>>>>>>> Either have

Re: PR merge policy

Violation of the PR merge policy is a sufficient reason to forcibly undo 
the commit and such violations affect everyone in the community.


Thank you,

Vlad

On 4/27/17 19:36, Pramod Immaneni wrote:

I agree that PRs should not be merged without a chance for others to
review. I don't agree to force push and altering the commit tree unless it
is absolutely needed, as it affects everyone. This case doesn't warrant
this step, one scenario where a force push might be needed is if somebody
pushed some copyrighted code.

Thanks

On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


I am open to both approaches - two commits or a join commit. Both have
pros and cons that we may discuss. What I am strongly against are PRs that
are merged without a chance for other contributors/committers to review.
There should be a way to forcibly undo such commits.

Thank you,

Vlad


On 4/27/17 12:41, Pramod Immaneni wrote:


My comments inline..

On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org> wrote:

I'm -1 on using the author field like this in Apex for the reason stated

(it is also odd to see something like this showing up without prior
discussion).


I am not set on this for future commits but would like to say, do we

really
verify the author field and treat it with importance. For example, I don't
think we ever check if the author is the person they say they are, check
name, email etc. If I were to use slightly different variations of my name
or email (not that I would do that) would reviewers really verify that. I
also have checked that tools don't fail on reading a commit because author
needs to be in a certain format. I guess contribution stats are the ones
that will be affected but if used rarely I dont see that being a big
problem. I can understand if we wanted to have strict requirements for the
committer field.

Thanks


Consider adding the additional author information to the commit message.

Thomas

On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
pra...@datatorrent.com>
wrote:

Agreed it is not regular and should only be used in special
circumstances.


One example of this is pair programming. It has been done before in
other
projects and searching on google or stackoverflow you can see how other
people have tried to handle it

https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
http://stackoverflow.com/questions/7442112/attributing-
a-single-commit-to-multiple-developers

Thanks

On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> wrote:

commit 9856080ede62a4529d730bcb6724c757f5010990

Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com>
Date:   Tue Apr 18 09:37:22 2017 -0700

Please don't use the author field in such a way, it leads to incorrect
tracking of contributions.

Either have separate commits or have one author.

Thanks



On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <


pra...@datatorrent.com
wrote:

The issue was two different plugin models were developed, one for

pre-launch and other for post-launch. I felt that the one built


latter

was

better and it would be better to have a uniform interface for the


users

and

hence asked for the changes.

On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org>


wrote:

I think the plugins feature could have benefited from better

original

review, which would have eliminated much of the back and forth

after

the

fact.


On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <


v.ro...@datatorrent.com

wrote:

Pramod,

No, it is not a request to update the apex.apache.org (to do


that

we


need

to file JIRA). It is a reminder that it is against Apex policy to


merge

PR

without giving enough time for others to review it (few hours


after

PR

was

open).

Thank you,

Vlad

On 4/27/17 08:05, Pramod Immaneni wrote:

Vlad, are you asking for a consensus on the policy to make it
official

(publish on website etc). I believe we have one. However, I did

not

see

much difference between what you said on Mar 26th to what I

proposed

on

Mar

24th. Is the main difference any committer can merge (not just


the

main

reviewer) as long as there are no objections from others. In

that

case,

I


am fine with it.

On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <


v.ro...@datatorrent.com>

wrote:

This is a friendly reminder regarding PR merge policy.


Thank you,

Vlad


On 3/23/17 12:58, Vlad Rozov wrote:

Lately there were few instances where PR open against apex-core


and

apex-malhar were merged just few hours after it being open and

JIRA

being

raised without giving chance for other contributors to review


and

comment.

I'd suggest that we stop such practice no matter how trivial


those

changes

are. This equally applies to documentation. In a rear cases


where

PR


is

urgent (for example one that fixes compilation error), I'd

suggest

that

a

c

Re: PR merge policy

t;>>>
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <
>>>>>>>
>>>>>> v.ro...@datatorrent.com
>>>
>>>> wrote:
>>>>>>>
>>>>>>> Pramod,
>>>>>>>>
>>>>>>>> No, it is not a request to update the apex.apache.org (to do
>>>>>>>>
>>>>>>> that
>>>
>>>> we
>>>>
>>>>> need
>>>>>>>
>>>>>>>> to file JIRA). It is a reminder that it is against Apex policy to
>>>>>>>>
>>>>>>> merge
>>>>>
>>>>>> PR
>>>>>>>
>>>>>>>> without giving enough time for others to review it (few hours
>>>>>>>>
>>>>>>> after
>>>
>>>> PR
>>>>>
>>>>>> was
>>>>>>>
>>>>>>>> open).
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>>
>>>>>>>> Vlad
>>>>>>>>
>>>>>>>> On 4/27/17 08:05, Pramod Immaneni wrote:
>>>>>>>>
>>>>>>>> Vlad, are you asking for a consensus on the policy to make it
>>>>>>>>>
>>>>>>>> official
>>>>>
>>>>>> (publish on website etc). I believe we have one. However, I did
>>>>>>>>>
>>>>>>>> not
>>>>
>>>>> see
>>>>>>
>>>>>>> much difference between what you said on Mar 26th to what I
>>>>>>>>>
>>>>>>>> proposed
>>>>
>>>>> on
>>>>>>
>>>>>>> Mar
>>>>>>>>> 24th. Is the main difference any committer can merge (not just
>>>>>>>>>
>>>>>>>> the
>>>
>>>> main
>>>>>>
>>>>>>> reviewer) as long as there are no objections from others. In
>>>>>>>>>
>>>>>>>> that
>>>
>>>> case,
>>>>>>
>>>>>>> I
>>>>>>>
>>>>>>>> am fine with it.
>>>>>>>>>
>>>>>>>>> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <
>>>>>>>>>
>>>>>>>> v.ro...@datatorrent.com>
>>>>>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> This is a friendly reminder regarding PR merge policy.
>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>>
>>>>>>>>>> Vlad
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 3/23/17 12:58, Vlad Rozov wrote:
>>>>>>>>>>
>>>>>>>>>> Lately there were few instances where PR open against apex-core
>>>>>>>>>>
>>>>>>>>> and
>>>>
>>>>> apex-malhar were merged just few hours after it being open and
>>>>>>>>>>>
>>>>>>>>>> JIRA
>>>>>
>>>>>> being
>>>>>>>>>>> raised without giving chance for other contributors to review
>>>>>>>>>>>
>>>>>>>>>> and
>>>>
>>>>> comment.
>>>>>>>>>>> I'd suggest that we stop such practice no matter how trivial
>>>>>>>>>>>
>>>>>>>>>> those
>>>>
>>>>> changes
>>>>>>>>>>> are. This equally applies to documentation. In a rear cases
>>>>>>>>>>>
>>>>>>>>>> where
>>>>
>>>>> PR
>>>>>
>>>>>> is
>>>>>>>
>>>>>>>> urgent (for example one that fixes compilation error), I'd
>>>>>>>>>>>
>>>>>>>>>> suggest
>>>>
>>>>> that
>>>>>>>
>>>>>>>> a
>>>>>>>>>>> committer who plans to merge the PR sends an explicit
>>>>>>>>>>>
>>>>>>>>>> notification
>>>>
>>>>> to
>>>>>>
>>>>>>> dev@apex and gives others a reasonable time to respond.
>>>>>>>>>>>
>>>>>>>>>>> Thank you,
>>>>>>>>>>>
>>>>>>>>>>> Vlad
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>

Re: PR merge policy

I am open to both approaches - two commits or a join commit. Both have 
pros and cons that we may discuss. What I am strongly against are PRs 
that are merged without a chance for other contributors/committers to 
review. There should be a way to forcibly undo such commits.


Thank you,

Vlad

On 4/27/17 12:41, Pramod Immaneni wrote:

My comments inline..

On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org> wrote:


I'm -1 on using the author field like this in Apex for the reason stated
(it is also odd to see something like this showing up without prior
discussion).



I am not set on this for future commits but would like to say, do we really
verify the author field and treat it with importance. For example, I don't
think we ever check if the author is the person they say they are, check
name, email etc. If I were to use slightly different variations of my name
or email (not that I would do that) would reviewers really verify that. I
also have checked that tools don't fail on reading a commit because author
needs to be in a certain format. I guess contribution stats are the ones
that will be affected but if used rarely I dont see that being a big
problem. I can understand if we wanted to have strict requirements for the
committer field.

Thanks



Consider adding the additional author information to the commit message.

Thomas

On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:


Agreed it is not regular and should only be used in special

circumstances.

One example of this is pair programming. It has been done before in other
projects and searching on google or stackoverflow you can see how other
people have tried to handle it

https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
http://stackoverflow.com/questions/7442112/attributing-
a-single-commit-to-multiple-developers

Thanks

On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> wrote:


commit 9856080ede62a4529d730bcb6724c757f5010990
Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com>
Date:   Tue Apr 18 09:37:22 2017 -0700

Please don't use the author field in such a way, it leads to incorrect
tracking of contributions.

Either have separate commits or have one author.

Thanks



On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <

pra...@datatorrent.com

wrote:


The issue was two different plugin models were developed, one for
pre-launch and other for post-launch. I felt that the one built

latter

was

better and it would be better to have a uniform interface for the

users

and

hence asked for the changes.

On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org>

wrote:

I think the plugins feature could have benefited from better

original

review, which would have eliminated much of the back and forth

after

the

fact.


On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <

v.ro...@datatorrent.com

wrote:


Pramod,

No, it is not a request to update the apex.apache.org (to do

that

we

need

to file JIRA). It is a reminder that it is against Apex policy to

merge

PR

without giving enough time for others to review it (few hours

after

PR

was

open).

Thank you,

Vlad

On 4/27/17 08:05, Pramod Immaneni wrote:


Vlad, are you asking for a consensus on the policy to make it

official

(publish on website etc). I believe we have one. However, I did

not

see

much difference between what you said on Mar 26th to what I

proposed

on

Mar
24th. Is the main difference any committer can merge (not just

the

main

reviewer) as long as there are no objections from others. In

that

case,

I

am fine with it.

On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <

v.ro...@datatorrent.com>

wrote:

This is a friendly reminder regarding PR merge policy.

Thank you,

Vlad


On 3/23/17 12:58, Vlad Rozov wrote:

Lately there were few instances where PR open against apex-core

and

apex-malhar were merged just few hours after it being open and

JIRA

being
raised without giving chance for other contributors to review

and

comment.
I'd suggest that we stop such practice no matter how trivial

those

changes
are. This equally applies to documentation. In a rear cases

where

PR

is

urgent (for example one that fixes compilation error), I'd

suggest

that

a
committer who plans to merge the PR sends an explicit

notification

to

dev@apex and gives others a reasonable time to respond.

Thank you,

Vlad

Re: PR merge policy

I'm -1 on using the author field like this in Apex for the reason stated
(it is also odd to see something like this showing up without prior
discussion).

Consider adding the additional author information to the commit message.

Thomas

On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:

> Agreed it is not regular and should only be used in special circumstances.
> One example of this is pair programming. It has been done before in other
> projects and searching on google or stackoverflow you can see how other
> people have tried to handle it
>
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
> http://stackoverflow.com/questions/7442112/attributing-
> a-single-commit-to-multiple-developers
>
> Thanks
>
> On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> wrote:
>
> > commit 9856080ede62a4529d730bcb6724c757f5010990
> > Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com>
> > Date:   Tue Apr 18 09:37:22 2017 -0700
> >
> > Please don't use the author field in such a way, it leads to incorrect
> > tracking of contributions.
> >
> > Either have separate commits or have one author.
> >
> > Thanks
> >
> >
> >
> > On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <pra...@datatorrent.com
> >
> > wrote:
> >
> > > The issue was two different plugin models were developed, one for
> > > pre-launch and other for post-launch. I felt that the one built latter
> > was
> > > better and it would be better to have a uniform interface for the users
> > and
> > > hence asked for the changes.
> > >
> > > On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org> wrote:
> > >
> > > > I think the plugins feature could have benefited from better original
> > > > review, which would have eliminated much of the back and forth after
> > the
> > > > fact.
> > > >
> > > >
> > > > On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <v.ro...@datatorrent.com
> >
> > > > wrote:
> > > >
> > > > > Pramod,
> > > > >
> > > > > No, it is not a request to update the apex.apache.org (to do that
> we
> > > > need
> > > > > to file JIRA). It is a reminder that it is against Apex policy to
> > merge
> > > > PR
> > > > > without giving enough time for others to review it (few hours after
> > PR
> > > > was
> > > > > open).
> > > > >
> > > > > Thank you,
> > > > >
> > > > > Vlad
> > > > >
> > > > > On 4/27/17 08:05, Pramod Immaneni wrote:
> > > > >
> > > > >> Vlad, are you asking for a consensus on the policy to make it
> > official
> > > > >> (publish on website etc). I believe we have one. However, I did
> not
> > > see
> > > > >> much difference between what you said on Mar 26th to what I
> proposed
> > > on
> > > > >> Mar
> > > > >> 24th. Is the main difference any committer can merge (not just the
> > > main
> > > > >> reviewer) as long as there are no objections from others. In that
> > > case,
> > > > I
> > > > >> am fine with it.
> > > > >>
> > > > >> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <
> > v.ro...@datatorrent.com>
> > > > >> wrote:
> > > > >>
> > > > >> This is a friendly reminder regarding PR merge policy.
> > > > >>>
> > > > >>> Thank you,
> > > > >>>
> > > > >>> Vlad
> > > > >>>
> > > > >>>
> > > > >>> On 3/23/17 12:58, Vlad Rozov wrote:
> > > > >>>
> > > > >>> Lately there were few instances where PR open against apex-core
> and
> > > > >>>> apex-malhar were merged just few hours after it being open and
> > JIRA
> > > > >>>> being
> > > > >>>> raised without giving chance for other contributors to review
> and
> > > > >>>> comment.
> > > > >>>> I'd suggest that we stop such practice no matter how trivial
> those
> > > > >>>> changes
> > > > >>>> are. This equally applies to documentation. In a rear cases
> where
> > PR
> > > > is
> > > > >>>> urgent (for example one that fixes compilation error), I'd
> suggest
> > > > that
> > > > >>>> a
> > > > >>>> committer who plans to merge the PR sends an explicit
> notification
> > > to
> > > > >>>> dev@apex and gives others a reasonable time to respond.
> > > > >>>>
> > > > >>>> Thank you,
> > > > >>>>
> > > > >>>> Vlad
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >
> > > >
> > >
> >
>

Re: PR merge policy

Agreed it is not regular and should only be used in special circumstances.
One example of this is pair programming. It has been done before in other
projects and searching on google or stackoverflow you can see how other
people have tried to handle it

https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
http://stackoverflow.com/questions/7442112/attributing-a-single-commit-to-multiple-developers

Thanks

On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> wrote:

> commit 9856080ede62a4529d730bcb6724c757f5010990
> Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com>
> Date:   Tue Apr 18 09:37:22 2017 -0700
>
> Please don't use the author field in such a way, it leads to incorrect
> tracking of contributions.
>
> Either have separate commits or have one author.
>
> Thanks
>
>
>
> On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <pra...@datatorrent.com>
> wrote:
>
> > The issue was two different plugin models were developed, one for
> > pre-launch and other for post-launch. I felt that the one built latter
> was
> > better and it would be better to have a uniform interface for the users
> and
> > hence asked for the changes.
> >
> > On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org> wrote:
> >
> > > I think the plugins feature could have benefited from better original
> > > review, which would have eliminated much of the back and forth after
> the
> > > fact.
> > >
> > >
> > > On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <v.ro...@datatorrent.com>
> > > wrote:
> > >
> > > > Pramod,
> > > >
> > > > No, it is not a request to update the apex.apache.org (to do that we
> > > need
> > > > to file JIRA). It is a reminder that it is against Apex policy to
> merge
> > > PR
> > > > without giving enough time for others to review it (few hours after
> PR
> > > was
> > > > open).
> > > >
> > > > Thank you,
> > > >
> > > > Vlad
> > > >
> > > > On 4/27/17 08:05, Pramod Immaneni wrote:
> > > >
> > > >> Vlad, are you asking for a consensus on the policy to make it
> official
> > > >> (publish on website etc). I believe we have one. However, I did not
> > see
> > > >> much difference between what you said on Mar 26th to what I proposed
> > on
> > > >> Mar
> > > >> 24th. Is the main difference any committer can merge (not just the
> > main
> > > >> reviewer) as long as there are no objections from others. In that
> > case,
> > > I
> > > >> am fine with it.
> > > >>
> > > >> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <
> v.ro...@datatorrent.com>
> > > >> wrote:
> > > >>
> > > >> This is a friendly reminder regarding PR merge policy.
> > > >>>
> > > >>> Thank you,
> > > >>>
> > > >>> Vlad
> > > >>>
> > > >>>
> > > >>> On 3/23/17 12:58, Vlad Rozov wrote:
> > > >>>
> > > >>> Lately there were few instances where PR open against apex-core and
> > > >>>> apex-malhar were merged just few hours after it being open and
> JIRA
> > > >>>> being
> > > >>>> raised without giving chance for other contributors to review and
> > > >>>> comment.
> > > >>>> I'd suggest that we stop such practice no matter how trivial those
> > > >>>> changes
> > > >>>> are. This equally applies to documentation. In a rear cases where
> PR
> > > is
> > > >>>> urgent (for example one that fixes compilation error), I'd suggest
> > > that
> > > >>>> a
> > > >>>> committer who plans to merge the PR sends an explicit notification
> > to
> > > >>>> dev@apex and gives others a reasonable time to respond.
> > > >>>>
> > > >>>> Thank you,
> > > >>>>
> > > >>>> Vlad
> > > >>>>
> > > >>>>
> > > >>>>
> > > >
> > >
> >
>

Re: PR merge policy

commit 9856080ede62a4529d730bcb6724c757f5010990
Author: Pramod Immaneni & Vlad Rozov <pramod+v.ro...@datatorrent.com>
Date:   Tue Apr 18 09:37:22 2017 -0700

Please don't use the author field in such a way, it leads to incorrect
tracking of contributions.

Either have separate commits or have one author.

Thanks



On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:

> The issue was two different plugin models were developed, one for
> pre-launch and other for post-launch. I felt that the one built latter was
> better and it would be better to have a uniform interface for the users and
> hence asked for the changes.
>
> On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org> wrote:
>
> > I think the plugins feature could have benefited from better original
> > review, which would have eliminated much of the back and forth after the
> > fact.
> >
> >
> > On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <v.ro...@datatorrent.com>
> > wrote:
> >
> > > Pramod,
> > >
> > > No, it is not a request to update the apex.apache.org (to do that we
> > need
> > > to file JIRA). It is a reminder that it is against Apex policy to merge
> > PR
> > > without giving enough time for others to review it (few hours after PR
> > was
> > > open).
> > >
> > > Thank you,
> > >
> > > Vlad
> > >
> > > On 4/27/17 08:05, Pramod Immaneni wrote:
> > >
> > >> Vlad, are you asking for a consensus on the policy to make it official
> > >> (publish on website etc). I believe we have one. However, I did not
> see
> > >> much difference between what you said on Mar 26th to what I proposed
> on
> > >> Mar
> > >> 24th. Is the main difference any committer can merge (not just the
> main
> > >> reviewer) as long as there are no objections from others. In that
> case,
> > I
> > >> am fine with it.
> > >>
> > >> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <v.ro...@datatorrent.com>
> > >> wrote:
> > >>
> > >> This is a friendly reminder regarding PR merge policy.
> > >>>
> > >>> Thank you,
> > >>>
> > >>> Vlad
> > >>>
> > >>>
> > >>> On 3/23/17 12:58, Vlad Rozov wrote:
> > >>>
> > >>> Lately there were few instances where PR open against apex-core and
> > >>>> apex-malhar were merged just few hours after it being open and JIRA
> > >>>> being
> > >>>> raised without giving chance for other contributors to review and
> > >>>> comment.
> > >>>> I'd suggest that we stop such practice no matter how trivial those
> > >>>> changes
> > >>>> are. This equally applies to documentation. In a rear cases where PR
> > is
> > >>>> urgent (for example one that fixes compilation error), I'd suggest
> > that
> > >>>> a
> > >>>> committer who plans to merge the PR sends an explicit notification
> to
> > >>>> dev@apex and gives others a reasonable time to respond.
> > >>>>
> > >>>> Thank you,
> > >>>>
> > >>>> Vlad
> > >>>>
> > >>>>
> > >>>>
> > >
> >
>

Re: PR merge policy


Pramod,

No, it is not a request to update the apex.apache.org (to do that we 
need to file JIRA). It is a reminder that it is against Apex policy to 
merge PR without giving enough time for others to review it (few hours 
after PR was open).


Thank you,

Vlad
On 4/27/17 08:05, Pramod Immaneni wrote:

Vlad, are you asking for a consensus on the policy to make it official
(publish on website etc). I believe we have one. However, I did not see
much difference between what you said on Mar 26th to what I proposed on Mar
24th. Is the main difference any committer can merge (not just the main
reviewer) as long as there are no objections from others. In that case, I
am fine with it.

On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:


This is a friendly reminder regarding PR merge policy.

Thank you,

Vlad


On 3/23/17 12:58, Vlad Rozov wrote:


Lately there were few instances where PR open against apex-core and
apex-malhar were merged just few hours after it being open and JIRA being
raised without giving chance for other contributors to review and comment.
I'd suggest that we stop such practice no matter how trivial those changes
are. This equally applies to documentation. In a rear cases where PR is
urgent (for example one that fixes compilation error), I'd suggest that a
committer who plans to merge the PR sends an explicit notification to
dev@apex and gives others a reasonable time to respond.

Thank you,

Vlad

Re: PR merge policy

Vlad, are you asking for a consensus on the policy to make it official
(publish on website etc). I believe we have one. However, I did not see
much difference between what you said on Mar 26th to what I proposed on Mar
24th. Is the main difference any committer can merge (not just the main
reviewer) as long as there are no objections from others. In that case, I
am fine with it.

On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

> This is a friendly reminder regarding PR merge policy.
>
> Thank you,
>
> Vlad
>
>
> On 3/23/17 12:58, Vlad Rozov wrote:
>
>> Lately there were few instances where PR open against apex-core and
>> apex-malhar were merged just few hours after it being open and JIRA being
>> raised without giving chance for other contributors to review and comment.
>> I'd suggest that we stop such practice no matter how trivial those changes
>> are. This equally applies to documentation. In a rear cases where PR is
>> urgent (for example one that fixes compilation error), I'd suggest that a
>> committer who plans to merge the PR sends an explicit notification to
>> dev@apex and gives others a reasonable time to respond.
>>
>> Thank you,
>>
>> Vlad
>>
>>
>

Re: PR merge policy