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
