+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 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.rozov@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 >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> -- >>>>>>> >>>>>>> _______________________________________________________ >>>>>>> >>>>>>> Munagala V. Ramanath >>>>>>> >>>>>>> Software Engineer >>>>>>> >>>>>>> E: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>>>>>> >>>>>>> www.datatorrent.com | apex.apache.org >>>>>>> >>>>>>> >>>>>>> -- >>>>> >>>>> _______________________________________________________ >>>>> >>>>> Munagala V. Ramanath >>>>> >>>>> Software Engineer >>>>> >>>>> E: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>>>> >>>>> www.datatorrent.com | apex.apache.org >>>>> >>>>> >>>>> >> >