Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 5BC2C200C76 for ; Sat, 29 Apr 2017 04:24:58 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 5A501160BB8; Sat, 29 Apr 2017 02:24:58 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 2A69D160BA3 for ; Sat, 29 Apr 2017 04:24:57 +0200 (CEST) Received: (qmail 20786 invoked by uid 500); 29 Apr 2017 02:24:54 -0000 Mailing-List: contact dev-help@apex.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@apex.apache.org Delivered-To: mailing list dev@apex.apache.org Received: (qmail 20770 invoked by uid 99); 29 Apr 2017 02:24:54 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 29 Apr 2017 02:24:54 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id 285121A0379 for ; Sat, 29 Apr 2017 02:24:54 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.99 X-Spam-Level: * X-Spam-Status: No, score=1.99 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, T_KAM_HTML_FONT_INVALID=0.01, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=datatorrent-com.20150623.gappssmtp.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id Z7qsNVVGuMEk for ; Sat, 29 Apr 2017 02:24:50 +0000 (UTC) Received: from mail-pg0-f46.google.com (mail-pg0-f46.google.com [74.125.83.46]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id 67D685F642 for ; Sat, 29 Apr 2017 02:24:49 +0000 (UTC) Received: by mail-pg0-f46.google.com with SMTP id v1so22715824pgv.1 for ; Fri, 28 Apr 2017 19:24:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=datatorrent-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=3EdkLJP3Q8/znvUZmD5CTEdRJs77ZTVFzslzVmmWb88=; b=nQxSd7/IIqoN4aaH0kYEOYNbIsrmfdZ8OLKiYKXkJp9usiw+LkX2eYx+JXjcsSqEB1 BIzJoZ2Wiym04QvwKLwYs65bafAEnjL06Q0B7RHXGLx9AzN3nR+Q2BZ8hnx0SxCpjTSS E70Fwv13k1O6bjDl71yAo1zfcahazA7o+VjDZYzLQBiyOcytk5Oybb99c0mJqPrD1dUr 3iqFrKPTFqUe1VgRk+IWefx5QtodZU9EHaitz8F8rHDviHN94+pe7cPcDRgNyvqcVtze /Q90WUHH9U99lrCuSm6A/sJzjSZHzsVb2bF5ktn/mtZYZSyB37dIuZlIBiMj4TY6A0xi kzyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=3EdkLJP3Q8/znvUZmD5CTEdRJs77ZTVFzslzVmmWb88=; b=iQ+Tu3QihSHESqddhe55gFvVBx/tdsXxtXVKxMUo71T6tcDPLjeLGVHaW7307CWL/G uwobQmgbyb6b5FcEtjHI3vMGqElcZ2Y21zFfjrZ5gVIW42o/k3OVEoBe3LcON6of8n2z /6NoV1UVHiLKxhKYBPsywdZPix3sHLDKC7oN/G+bsG8Xe861u0vlQq6NMAttjizGhbR3 3LOC8bFJUdp13wUJjrqfNBHCjlv9NMAzLqccP4LFdq7gYe0ax6gu7FewycW7Hb/VN1u3 UwCzSCmCw99YC1ir4+rxZJHqPYdUlh5G+UvRSOxz61e1wM2hTsW9zMEpgNSWADsKJw44 LnqQ== X-Gm-Message-State: AN3rC/6JfKKc6jvqpfxZZdj2LSANkxVQR9KFGaO/6zdGCO7/3LCCIGAD P72zdpNkDoxFYp3xy7loLSQHgoY2gGZn X-Received: by 10.99.140.88 with SMTP id q24mr15226845pgn.237.1493432688190; Fri, 28 Apr 2017 19:24:48 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.153.197 with HTTP; Fri, 28 Apr 2017 19:24:47 -0700 (PDT) In-Reply-To: References: <7eb5beef-1f37-860f-276e-ebc290cffddd@datatorrent.com> <32fd73a6-3f1e-33dd-c64e-113969c0f3b1@datatorrent.com> <-417195641274065270@unknownmsgid> From: Amol Kekre Date: Fri, 28 Apr 2017 19:24:47 -0700 Message-ID: Subject: Re: PR merge policy To: dev@apex.apache.org Content-Type: multipart/alternative; boundary=94eb2c1b61cce03e6b054e44e4fd archived-at: Sat, 29 Apr 2017 02:24:58 -0000 --94eb2c1b61cce03e6b054e44e4fd Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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:amol@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre* www.datatorrent.com On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov wrote= : > I think it is a good idea to make the committer responsible for fixing th= e > 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 opportuniti= es >> 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. W= e >> need to be cognizant of new contributors worrying about the cost to subm= it >> code. >> >> I too do not think Apex is hurting from bad code getting in. We are doin= g >> great with our current policies. >> >> Thks, >> Amol >> >> >> E:amol@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre* >> >> www.datatorrent.com >> >> >> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya < >> Ilya.Ganelin@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 a= nd >>> then allowing them to resolve it. E.g. I improperly commit an unreviewe= d >>> 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=E2= =80=99re >>> 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=E2=80=99ve had great success with a policy r= equiring >>> 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 hel= ps >>> to build collaboration, familiarity with the code base, and intrinsical= ly >>> avoids PRs being merged too quickly (without a sufficient period for >>> review). >>> >>> >>> >>> >>> >>> - Ilya Ganelin >>> >>> [image: id:image001.png@01D1F7A4.F3D42980] >>> >>> >>> >>> *From: *Pramod Immaneni >>> *Reply-To: *"dev@apex.apache.org" >>> *Date: *Friday, April 28, 2017 at 10:09 AM >>> *To: *"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 >>> 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 wrote: >>> >>> +1 as measure of last resort. >>> >>> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov >>> wrote: >>> >>> IMO, force push will bring enough consequent embarrassment to avoid suc= h >>> 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 reminde= r >>> 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 >>> wrote: >>> >>> I also was under impression that everyone agreed to the policy that giv= es >>> >>> 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 violation= s. >>> >>> 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 fir= st >>> >>> 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 >> 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 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 < >>> ram@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 >> 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.rozov@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 >>> >>> 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 < >>> >>> pramod@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=3D375536 >>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=3D451880 >>> http://stackoverflow.com/questions/7442112/attributing- >>> a-single-commit-to-multiple-developers >>> >>> Thanks >>> >>> On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise >>> >>> wrote: >>> >>> commit 9856080ede62a4529d730bcb6724c757f5010990 >>> >>> Author: Pramod Immaneni & Vlad Rozov >>> >>> >> >>> 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 < >>> >>> pramod@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 >> >>> 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.rozov@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.rozov@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: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>> >>> www.datatorrent.com | apex.apache.org >>> >>> >>> -- >>> >>> _______________________________________________________ >>> >>> Munagala V. Ramanath >>> >>> Software Engineer >>> >>> E: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>> >>> www.datatorrent.com | apex.apache.org >>> >>> >>> >>> >>> >>> >>> ------------------------------ >>> >>> The information contained in this e-mail is confidential and/or >>> proprietary to Capital One and/or its affiliates and may only be used >>> solely in performance of work or services for Capital One. The >>> information >>> transmitted herewith is intended only for use by the individual or enti= ty >>> to which it is addressed. If the reader of this message is not the >>> intended >>> recipient, you are hereby notified that any review, retransmission, >>> dissemination, distribution, copying or other use of, or taking of any >>> action in reliance upon this information is strictly prohibited. If you >>> have received this communication in error, please contact the sender an= d >>> delete the material from your computer. >>> >>> > --94eb2c1b61cce03e6b054e44e4fd--