You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by Otto Fowler <ot...@gmail.com> on 2017/09/20 12:19:43 UTC
feature branch bumps
Can I get a bump on https://github.com/apache/metron/pull/747 and
https://github.com/apache/metron/pull/761?
The next time I take master is going to be a bit of integration work, and I
would like to unstack and get my stuff integrated first.
Re: feature branch bumps
Posted by "Zeolla@GMail.com" <ze...@gmail.com>.
But wait, I thought we had established that this was such a fundamental
change that it was hard to chunk it out and keep master working.
Jon
On Wed, Sep 20, 2017 at 3:08 PM Nick Allen <ni...@nickallen.org> wrote:
> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>
> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>
> > Nick: Are you going to bite-off small chunks of the feature branch and
> introduce PRs against master?
>
>
>
>
>
> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
> > Well, if there is an alternative merge strategy, I’m all ears.
> > As it stands, the only complete code / conceptual review has been the
> > bundles-lib by mattf.
> > So there is still a great deal of review activity to do.
> >
> > Also conceptually there is ( might not be complete )
> >
> > * the discuss thread topic from this morning ( metron parsers v.
> > extensions wrt registration and management )
> > * default configurations ( parser, enrichment, indexing, elasticsearch)
> >
> >
> >
> >
> > On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
> wrote:
> >
> > Ok, so it sounds like you are envisioning a "big bang" merge between the
> > feature branch and master at some point. Not ideal in my mind, but maybe
> > we need to be more pragmatic with this one.
> >
> > Do we have other tasks (like these PRs) to complete before we are ready
> to
> > consider the merge?
> >
> > I am just looking to help however I can in killing this feature branch;
> > meaning I want your code in master, as soon as possible. :)
> >
> >
> >
> >
> >
> >
> > On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
> > wrote:
> >
> >> So, were the functionality is not FB related, I have been doing PR’s
> >> against master ( such as hdfs service ability to set permissions ).
> >> I don’t think we have talked about the end game PR from feature to
> >> master, I am don’t know how we would do multiple PRs to bring it down
> >> once ‘accepted’. I think that approach needs to to be discussed and
> >> agreed on.
> >>
> >>
> >> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
> wrote:
> >>
> >> So it sounds like these PRs do move us closer to bringing the two
> >> branches together. But I think I am missing your high-level approach
> >> though.
> >>
> >> How are we going to get all of the functionality from the feature branch
> >> into master? Are you going to bite-off small chunks of the feature
> branch
> >> and introduce PRs against master?
> >>
> >>
> >>
> >> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
> >> wrote:
> >>
> >>> Right now, these two PR’s are stacked, the UI depends on the
> >>> BundleSystem changes.
> >>> I would rather bring them down to feature before I do master
> >>> integration, than integrate master
> >>> into feature and then bring it out to the stacked PR’s. Simple as
> that.
> >>>
> >>>
> >>>
> >>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
> >>> wrote:
> >>>
> >>> Hi Otto -
> >>>
> >>> What is the plan for bringing the feature branch and master together?
> Do
> >>> these PRs move us closer to bringing the two branches together?
> >>>
> >>> Thanks
> >>>
> >>>
> >>>
> >>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
> >>> wrote:
> >>>
> >>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
> >>> > https://github.com/apache/metron/pull/761?
> >>> >
> >>> > The next time I take master is going to be a bit of integration work,
> >>> and I
> >>> > would like to unstack and get my stuff integrated first.
> >>> >
> >>>
> >>>
>
--
Jon
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
Omit
On September 20, 2017 at 19:20:27, Otto Fowler (ottobackwards@gmail.com)
wrote:
> With the bundles, archetype, plug-in and parser moves gone 777 will be
> much smaller though.
>
> Also I did not mean to admit Mike’s doc reviews, sorry.
>
> On September 20, 2017 at 18:04:20, Otto Fowler (ottobackwards@gmail.com)
> wrote:
>
>> I am not taking any of this as starting to do a flame war :) And your
>> concern is about review not merging right? If it is all reviewed and
>> accepted, why would 6 merges be preferable? That doesn’t make sense.
>> Breaking it up only makes sense with regards to review, so that is how
>> I’ll take it.
>>
>> And I *am* all ears for how to break up the code. Like literally HOW.
>> Like : ‘I would diff a folder with master, create a new branch for the pr
>> and patch just that’.
>> Just 'break up the massive working thing into smaller chucks' is not
>> enough for my relative experience with git and github to you Nick.
>>
>> So if all we want are code review chunks, regardless of being able to run
>> them and see them work then ( but also not break the regular build and
>> full_dev ):
>>
>> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
>> code review only, doesn’t do anything
>> 2. PR for Archetype ( just building the result, unit integration tests )
>> code review and generate project review. projects built by archetype :
>> => not buildable not installable not testable
>> relies on changes to parser testing etc. to work with non-hard coded
>> paths for test resources etc etc
>> 3. PR with Parsers copied to extensions
>> code review only. not buildable not installable not testable
>> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move
>> to new parser structure, load install, grok support etc etc.
>> regression testable, status quo ante functionality.
>> still huge
>> will still be a problem
>> cannot be broken down and built / pass travis at this point
>> this is the integration
>> 5. PR of Rest
>> can install and uninstall via rest
>> 6. PR of UI
>> can list, install uninstall via UI
>>
>> I did not think then that we wanted PRs that don’t build or test or run
>> the changes they have ( but the branch builds and passes travis ), but just
>> have code to review.
>>
>> But, as you said, it has been since April, and only Bundles has been
>> reviewed. If nobody but Matt is even going to _attempt_ code review, then
>> it may now be implicitly required that I do this.
>>
>>
>> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>>
>> I was just responding to your statement "Well, if there is an
>> alternative merge strategy, I’m all ears." I understand that you disagree,
>> but this is an alternative strategy. I think this approach is feasible and
>> I outlined how here [1].
>>
>> The criticism of this approach is the amount of effort and time to do
>> this, which I agree with. It will take a good deal of effort.
>> I acquiesced to this criticism at the time because I thought we were trying
>> to push this change in quickly. But it has been a couple months now and we
>> still have a bunch of code to merge.
>>
>> This is no criticism of you or anyone here. What Otto has been doing in
>> keeping this branch merged with master is god damn heroic. And I am so
>> excited to see this functionality hit master. I just wanted to brainstorm
>> on ideas to help get this in. Right now, I am unsure of the path and
>> timeline on how we are going to get this merged.
>>
>> Please read this in the kindest possible light. I am not trying to start
>> a flame war or offend anyone.
>>
>>
>> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>>
>>
>>
>> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>>> that from an already integrated and working branch.
>>> Do you mean I…
>>> - create patch files of whole directories and do a pr per directory, but
>>> the build doesn’t work?
>>>
>>>
>>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>>
>>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>>
>>> > Nick: Are you going to bite-off small chunks of the feature branch
>>> and introduce PRs against master?
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> Well, if there is an alternative merge strategy, I’m all ears.
>>>> As it stands, the only complete code / conceptual review has been the
>>>> bundles-lib by mattf.
>>>> So there is still a great deal of review activity to do.
>>>>
>>>> Also conceptually there is ( might not be complete )
>>>>
>>>> * the discuss thread topic from this morning ( metron parsers v.
>>>> extensions wrt registration and management )
>>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>>
>>>>
>>>>
>>>>
>>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> Ok, so it sounds like you are envisioning a "big bang" merge between
>>>> the feature branch and master at some point. Not ideal in my mind, but
>>>> maybe we need to be more pragmatic with this one.
>>>>
>>>> Do we have other tasks (like these PRs) to complete before we are ready
>>>> to consider the merge?
>>>>
>>>> I am just looking to help however I can in killing this feature branch;
>>>> meaning I want your code in master, as soon as possible. :)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>>> against master ( such as hdfs service ability to set permissions ).
>>>>> I don’t think we have talked about the end game PR from feature to
>>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>>> agreed on.
>>>>>
>>>>>
>>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> So it sounds like these PRs do move us closer to bringing the two
>>>>> branches together. But I think I am missing your high-level approach
>>>>> though.
>>>>>
>>>>> How are we going to get all of the functionality from the feature
>>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>>> branch and introduce PRs against master?
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>>> BundleSystem changes.
>>>>>> I would rather bring them down to feature before I do master
>>>>>> integration, than integrate master
>>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>>> that.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>>> wrote:
>>>>>>
>>>>>> Hi Otto -
>>>>>>
>>>>>> What is the plan for bringing the feature branch and master together?
>>>>>> Do
>>>>>> these PRs move us closer to bringing the two branches together?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>>> > https://github.com/apache/metron/pull/761?
>>>>>> >
>>>>>> > The next time I take master is going to be a bit of integration
>>>>>> work, and I
>>>>>> > would like to unstack and get my stuff integrated first.
>>>>>> >
>>>>>>
>>>>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
With the bundles, archetype, plug-in and parser moves gone 777 will be much
smaller though.
Also I did not mean to admit Mike’s doc reviews, sorry.
On September 20, 2017 at 18:04:20, Otto Fowler (ottobackwards@gmail.com)
wrote:
> I am not taking any of this as starting to do a flame war :) And your
> concern is about review not merging right? If it is all reviewed and
> accepted, why would 6 merges be preferable? That doesn’t make sense.
> Breaking it up only makes sense with regards to review, so that is how
> I’ll take it.
>
> And I *am* all ears for how to break up the code. Like literally HOW.
> Like : ‘I would diff a folder with master, create a new branch for the pr
> and patch just that’.
> Just 'break up the massive working thing into smaller chucks' is not
> enough for my relative experience with git and github to you Nick.
>
> So if all we want are code review chunks, regardless of being able to run
> them and see them work then ( but also not break the regular build and
> full_dev ):
>
> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
> code review only, doesn’t do anything
> 2. PR for Archetype ( just building the result, unit integration tests )
> code review and generate project review. projects built by archetype :
> => not buildable not installable not testable
> relies on changes to parser testing etc. to work with non-hard coded paths
> for test resources etc etc
> 3. PR with Parsers copied to extensions
> code review only. not buildable not installable not testable
> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move to
> new parser structure, load install, grok support etc etc.
> regression testable, status quo ante functionality.
> still huge
> will still be a problem
> cannot be broken down and built / pass travis at this point
> this is the integration
> 5. PR of Rest
> can install and uninstall via rest
> 6. PR of UI
> can list, install uninstall via UI
>
> I did not think then that we wanted PRs that don’t build or test or run
> the changes they have ( but the branch builds and passes travis ), but just
> have code to review.
>
> But, as you said, it has been since April, and only Bundles has been
> reviewed. If nobody but Matt is even going to _attempt_ code review, then
> it may now be implicitly required that I do this.
>
>
> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>
> I was just responding to your statement "Well, if there is an alternative
> merge strategy, I’m all ears." I understand that you disagree, but this is
> an alternative strategy. I think this approach is feasible and I outlined
> how here [1].
>
> The criticism of this approach is the amount of effort and time to do
> this, which I agree with. It will take a good deal of effort.
> I acquiesced to this criticism at the time because I thought we were trying
> to push this change in quickly. But it has been a couple months now and we
> still have a bunch of code to merge.
>
> This is no criticism of you or anyone here. What Otto has been doing in
> keeping this branch merged with master is god damn heroic. And I am so
> excited to see this functionality hit master. I just wanted to brainstorm
> on ideas to help get this in. Right now, I am unsure of the path and
> timeline on how we are going to get this merged.
>
> Please read this in the kindest possible light. I am not trying to start
> a flame war or offend anyone.
>
>
> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>
>
>
> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>> that from an already integrated and working branch.
>> Do you mean I…
>> - create patch files of whole directories and do a pr per directory, but
>> the build doesn’t work?
>>
>>
>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
>>
>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>
>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>
>> > Nick: Are you going to bite-off small chunks of the feature branch and
>> introduce PRs against master?
>>
>>
>>
>>
>>
>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> Well, if there is an alternative merge strategy, I’m all ears.
>>> As it stands, the only complete code / conceptual review has been the
>>> bundles-lib by mattf.
>>> So there is still a great deal of review activity to do.
>>>
>>> Also conceptually there is ( might not be complete )
>>>
>>> * the discuss thread topic from this morning ( metron parsers v.
>>> extensions wrt registration and management )
>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>
>>>
>>>
>>>
>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> Ok, so it sounds like you are envisioning a "big bang" merge between the
>>> feature branch and master at some point. Not ideal in my mind, but maybe
>>> we need to be more pragmatic with this one.
>>>
>>> Do we have other tasks (like these PRs) to complete before we are ready
>>> to consider the merge?
>>>
>>> I am just looking to help however I can in killing this feature branch;
>>> meaning I want your code in master, as soon as possible. :)
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>> against master ( such as hdfs service ability to set permissions ).
>>>> I don’t think we have talked about the end game PR from feature to
>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>> agreed on.
>>>>
>>>>
>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> So it sounds like these PRs do move us closer to bringing the two
>>>> branches together. But I think I am missing your high-level approach
>>>> though.
>>>>
>>>> How are we going to get all of the functionality from the feature
>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>> branch and introduce PRs against master?
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>> BundleSystem changes.
>>>>> I would rather bring them down to feature before I do master
>>>>> integration, than integrate master
>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>> that.
>>>>>
>>>>>
>>>>>
>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> Hi Otto -
>>>>>
>>>>> What is the plan for bringing the feature branch and master together?
>>>>> Do
>>>>> these PRs move us closer to bringing the two branches together?
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>> > https://github.com/apache/metron/pull/761?
>>>>> >
>>>>> > The next time I take master is going to be a bit of integration
>>>>> work, and I
>>>>> > would like to unstack and get my stuff integrated first.
>>>>> >
>>>>>
>>>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
My thought was that in answering things in the wiki, it would build out the
‘guide’ there. But I was just taking a stab at that. I am open to
whatever the group thinks would work best.
On September 21, 2017 at 14:47:10, Nick Allen (nick@nickallen.org) wrote:
> What I would like to do, and what it seems to me that we need to do (
again I apologize if I am wrong ), is to catch up on the confluence and
where things currently stand, and then move the discussion on from there.
Perfect, thanks. I think you are completely right. I will review that in
detail.
Should I respond with questions in the comments of the Wiki or some other
way? Whatever works for you.
On Thu, Sep 21, 2017 at 2:24 PM Otto Fowler <ot...@gmail.com> wrote:
> Nick, thanks for taking the time to reply, I have no doubts about your
> motivations and intent. Hopefully we get through this point by point
> stuff, which is always difficult on email.
>
>
> https://cwiki.apache.org/confluence/display/METRON/Metron+Extension+System+and+Parser+Extensions
>
> This is my recent attempt at creating a framework for the review. It
> includes testing areas, review areas etc. I have not received any feedback
> on it so I have to assume that
> you have not seen it. My stated hope is that through questions about the
> confluence I can address anything missing to make things reviewable, or
> more reviewable. If you have reviewed it, and are just not referring to it,
> I apologize.
> Please take a look, and provide feedback so that I can address it there if
> you have not.
>
> As far as the problem being lack of community commitment, that is not what
> I was trying to imply. I was, in my mind agreeing with you that the size
> of the changes has proven to be a barrier in practice, despite our
> agreement in 777 to proceed as is, and our discussion about the feature
> branch.
>
> As far as (1) the scope, I tried to lay out the areas of change and what
> was changed in the description of 777. I also re-listed separately the
> specific files changed, and what functional area the change referred to in
> the PR. Can you point me to an example of a different definition of scope
> that I can copy? Is it a more descriptive but still high level statement
> that would help?
>
> As far as (2) I have outlined the parts that could be considered
> separately ( bundles, archetype, maven plugin ). Once we switch to the new
> parser structure, there is no way to cut it down and still have a working
> build, because the new parser structure and loading touches pretty much all
> levels. This is not that different from what we discussed at the time. I
> do not know what is in there that should not be in there, aside from the 3
> areas I mentioned that could be reviewed as not integrated components. Are
> you referring to those areas or something else?
>
> As far as (3) - In the original 777, there was no user facing changes.
> Only developer changes, which I addressed in the adding system parsers
> guide based on Mike’s feedback. Maybe we are using different terminology
> and just missing each other. Based on the original 777, a user ( someone
> who deploys metron and uses the ambari configuration and the management ui,
> pushes new parsers to zookeeper, uses kibana etc) would not see any
> changes, or should not have seen any changes. There was no new
> functionality to test from a user POV. Mike did find a regression in the
> config ui wrt grok rules editing, which I addressed, but that was not new
> function problem, but a regression. The user facing changes where in 942 (
> rest + stellar management command) and where documented, and are also now
> in the UI pr. Asking how a user creates a parser in 777 when I
> specifically put that in 942 to cut down on scope, while also questioning
> the scope is confusing to me.
>
> As far as (4) - please see the confluence pages.
>
> What I would like to do, and what it seems to me that we need to do (
> again I apologize if I am wrong ), is to catch up on the confluence and
> where things currently stand, and then move the discussion on from there.
> I feel like a lot has gone on in 777 and there that relate to your concerns
> ( although I am not saying they address them ).
>
>
> On September 21, 2017 at 11:58:51, Nick Allen (nick@nickallen.org) wrote:
>
> But, as you said, it has been since April, and only Bundles has been
>> reviewed. If nobody but Matt is even going to _attempt_ code review, then
>> it may now be implicitly required that I do this.
>
>
> I disagree
> that the problem is lack of community commitment to the change
> . We have had quite a few core team members who at least began a review. I
> can't speak for anyone else, but I'll speak for my own experience
> in attempting to review 777.
>
> These are the reasons why I was not able to close out my review of 777
> with a +1.
>
> (1)
>
> T
> he scope of this change is largely undefined.
> I do
> not
>
> have
> a good understanding
>
> of what this thing actually does
>
> and does not do
> . What does it touch and what does it not touch?
>
>
> (2)
> I feel like there are lots of different pieces of functionality
>
> in the FB
> that we've tied all together that doesn't necessarily need to be.
>
> With this being one big
>
> chunk of code, its take it all or nothing.
>
>
> (3)
> I asked for documentation on how a user is supposed to interact with
> this change. This should help define what a parser is, how a user creates
> a parser, does an analyst have to run Maven just to deploy a parser?
>
>
>
> (4)
> I asked for a complete test plan on how to test all of this
> functionality.
>
>
> I don't think I received for very good responses from you on these
> questions. Comments from you like this [1], do not help me review the code.
>
> Since this PR is not adding new functionality the testing is the usual
>> smoke test for parser functionality per functional area ( Full Dev has
>> data, Metron UI sees the configurations ). The existing parsers should
>> work, you should not notice a difference.
>
>
>
>> I feel this PR delivers the things discussed on list, and as described in
>> the status updates
>
>
> Clearly this amount of code does not result in no net-new functionality.
> A usual smoke test is just not good enough here.
> You can't refer to requirements that have been made on the mailing list.
> That is not sufficient.
> But m
> aybe th
> is
> has
> all
> changed since my first review attempt. If so, please point me
> to where this is documented
> and I will take a look.
>
> You can see
>
> a symptom of
> these problems in
> the discuss thread that you opened yesterday. We don't have agreement in
> the community on basic definitions like what a parser is. That's not a
> good sign.
>
>
> I hope this helps you understand (at least my view point of) some of the
> problems in reviewing such a large PR. Even if we break this up into
> smaller PRs, we still need the same things. What is the scope? What does
> this touch? How is a user going to use this? How do I test this? It is
> just much easier to answer these questions on a smaller PR.
>
> I provide this feedback with the highest level of respect for what you've
> been able to accomplish. I just want to provide candid feedback to perhaps
> help push this over the finish line.
>
>
> [1] https://github.com/apache/metron/pull/530#issuecomment-306604503
>
>
>
>
>
>
>
> On Wed, Sep 20, 2017 at 6:04 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> I am not taking any of this as starting to do a flame war :) And your
>> concern is about review not merging right? If it is all reviewed and
>> accepted, why would 6 merges be preferable? That doesn’t make sense.
>> Breaking it up only makes sense with regards to review, so that is how
>> I’ll take it.
>>
>> And I *am* all ears for how to break up the code. Like literally HOW.
>> Like : ‘I would diff a folder with master, create a new branch for the pr
>> and patch just that’.
>> Just 'break up the massive working thing into smaller chucks' is not
>> enough for my relative experience with git and github to you Nick.
>>
>> So if all we want are code review chunks, regardless of being able to run
>> them and see them work then ( but also not break the regular build and
>> full_dev ):
>>
>> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
>> code review only, doesn’t do anything
>> 2. PR for Archetype ( just building the result, unit integration tests )
>> code review and generate project review. projects built by archetype :
>> => not buildable not installable not testable
>> relies on changes to parser testing etc. to work with non-hard coded
>> paths for test resources etc etc
>> 3. PR with Parsers copied to extensions
>> code review only. not buildable not installable not testable
>> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move
>> to new parser structure, load install, grok support etc etc.
>> regression testable, status quo ante functionality.
>> still huge
>> will still be a problem
>> cannot be broken down and built / pass travis at this point
>> this is the integration
>> 5. PR of Rest
>> can install and uninstall via rest
>> 6. PR of UI
>> can list, install uninstall via UI
>>
>> I did not think then that we wanted PRs that don’t build or test or run
>> the changes they have ( but the branch builds and passes travis ), but just
>> have code to review.
>>
>> But, as you said, it has been since April, and only Bundles has been
>> reviewed. If nobody but Matt is even going to _attempt_ code review, then
>> it may now be implicitly required that I do this.
>>
>>
>> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>>
>> I was just responding to your statement "Well, if there is an
>> alternative merge strategy, I’m all ears." I understand that you disagree,
>> but this is an alternative strategy. I think this approach is feasible and
>> I outlined how here [1].
>>
>> The criticism of this approach is the amount of effort and time to do
>> this, which I agree with. It will take a good deal of effort.
>> I acquiesced to this criticism at the time because I thought we were trying
>> to push this change in quickly. But it has been a couple months now and we
>> still have a bunch of code to merge.
>>
>> This is no criticism of you or anyone here. What Otto has been doing in
>> keeping this branch merged with master is god damn heroic. And I am so
>> excited to see this functionality hit master. I just wanted to brainstorm
>> on ideas to help get this in. Right now, I am unsure of the path and
>> timeline on how we are going to get this merged.
>>
>> Please read this in the kindest possible light. I am not trying to start
>> a flame war or offend anyone.
>>
>>
>> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>>
>>
>>
>> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>>> that from an already integrated and working branch.
>>> Do you mean I…
>>> - create patch files of whole directories and do a pr per directory, but
>>> the build doesn’t work?
>>>
>>>
>>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>>
>>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>>
>>> > Nick: Are you going to bite-off small chunks of the feature branch
>>> and introduce PRs against master?
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> Well, if there is an alternative merge strategy, I’m all ears.
>>>> As it stands, the only complete code / conceptual review has been the
>>>> bundles-lib by mattf.
>>>> So there is still a great deal of review activity to do.
>>>>
>>>> Also conceptually there is ( might not be complete )
>>>>
>>>> * the discuss thread topic from this morning ( metron parsers v.
>>>> extensions wrt registration and management )
>>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>>
>>>>
>>>>
>>>>
>>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> Ok, so it sounds like you are envisioning a "big bang" merge between
>>>> the feature branch and master at some point. Not ideal in my mind, but
>>>> maybe we need to be more pragmatic with this one.
>>>>
>>>> Do we have other tasks (like these PRs) to complete before we are ready
>>>> to consider the merge?
>>>>
>>>> I am just looking to help however I can in killing this feature branch;
>>>> meaning I want your code in master, as soon as possible. :)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>>> against master ( such as hdfs service ability to set permissions ).
>>>>> I don’t think we have talked about the end game PR from feature to
>>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>>> agreed on.
>>>>>
>>>>>
>>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> So it sounds like these PRs do move us closer to bringing the two
>>>>> branches together. But I think I am missing your high-level approach
>>>>> though.
>>>>>
>>>>> How are we going to get all of the functionality from the feature
>>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>>> branch and introduce PRs against master?
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>>> BundleSystem changes.
>>>>>> I would rather bring them down to feature before I do master
>>>>>> integration, than integrate master
>>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>>> that.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>>> wrote:
>>>>>>
>>>>>> Hi Otto -
>>>>>>
>>>>>> What is the plan for bringing the feature branch and master together?
>>>>>> Do
>>>>>> these PRs move us closer to bringing the two branches together?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>>> > https://github.com/apache/metron/pull/761?
>>>>>> >
>>>>>> > The next time I take master is going to be a bit of integration
>>>>>> work, and I
>>>>>> > would like to unstack and get my stuff integrated first.
>>>>>> >
>>>>>>
>>>>>>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
> What I would like to do, and what it seems to me that we need to do (
again I apologize if I am wrong ), is to catch up on the confluence and
where things currently stand, and then move the discussion on from there.
Perfect, thanks. I think you are completely right. I will review that in
detail.
Should I respond with questions in the comments of the Wiki or some other
way? Whatever works for you.
On Thu, Sep 21, 2017 at 2:24 PM Otto Fowler <ot...@gmail.com> wrote:
> Nick, thanks for taking the time to reply, I have no doubts about your
> motivations and intent. Hopefully we get through this point by point
> stuff, which is always difficult on email.
>
>
> https://cwiki.apache.org/confluence/display/METRON/Metron+Extension+System+and+Parser+Extensions
>
> This is my recent attempt at creating a framework for the review. It
> includes testing areas, review areas etc. I have not received any feedback
> on it so I have to assume that
> you have not seen it. My stated hope is that through questions about the
> confluence I can address anything missing to make things reviewable, or
> more reviewable. If you have reviewed it, and are just not referring to it,
> I apologize.
> Please take a look, and provide feedback so that I can address it there if
> you have not.
>
> As far as the problem being lack of community commitment, that is not what
> I was trying to imply. I was, in my mind agreeing with you that the size
> of the changes has proven to be a barrier in practice, despite our
> agreement in 777 to proceed as is, and our discussion about the feature
> branch.
>
> As far as (1) the scope, I tried to lay out the areas of change and what
> was changed in the description of 777. I also re-listed separately the
> specific files changed, and what functional area the change referred to in
> the PR. Can you point me to an example of a different definition of scope
> that I can copy? Is it a more descriptive but still high level statement
> that would help?
>
> As far as (2) I have outlined the parts that could be considered
> separately ( bundles, archetype, maven plugin ). Once we switch to the new
> parser structure, there is no way to cut it down and still have a working
> build, because the new parser structure and loading touches pretty much all
> levels. This is not that different from what we discussed at the time. I
> do not know what is in there that should not be in there, aside from the 3
> areas I mentioned that could be reviewed as not integrated components. Are
> you referring to those areas or something else?
>
> As far as (3) - In the original 777, there was no user facing changes.
> Only developer changes, which I addressed in the adding system parsers
> guide based on Mike’s feedback. Maybe we are using different terminology
> and just missing each other. Based on the original 777, a user ( someone
> who deploys metron and uses the ambari configuration and the management ui,
> pushes new parsers to zookeeper, uses kibana etc) would not see any
> changes, or should not have seen any changes. There was no new
> functionality to test from a user POV. Mike did find a regression in the
> config ui wrt grok rules editing, which I addressed, but that was not new
> function problem, but a regression. The user facing changes where in 942 (
> rest + stellar management command) and where documented, and are also now
> in the UI pr. Asking how a user creates a parser in 777 when I
> specifically put that in 942 to cut down on scope, while also questioning
> the scope is confusing to me.
>
> As far as (4) - please see the confluence pages.
>
> What I would like to do, and what it seems to me that we need to do (
> again I apologize if I am wrong ), is to catch up on the confluence and
> where things currently stand, and then move the discussion on from there.
> I feel like a lot has gone on in 777 and there that relate to your concerns
> ( although I am not saying they address them ).
>
>
> On September 21, 2017 at 11:58:51, Nick Allen (nick@nickallen.org) wrote:
>
> But, as you said, it has been since April, and only Bundles has been
>> reviewed. If nobody but Matt is even going to _attempt_ code review, then
>> it may now be implicitly required that I do this.
>
>
> I disagree
> that the problem is lack of community commitment to the change
> . We have had quite a few core team members who at least began a review. I
> can't speak for anyone else, but I'll speak for my own experience
> in attempting to review 777.
>
> These are the reasons why I was not able to close out my review of 777
> with a +1.
>
> (1)
>
> T
> he scope of this change is largely undefined.
> I do
> not
>
> have
> a good understanding
>
> of what this thing actually does
>
> and does not do
> . What does it touch and what does it not touch?
>
>
> (2)
> I feel like there are lots of different pieces of functionality
>
> in the FB
> that we've tied all together that doesn't necessarily need to be.
>
> With this being one big
>
> chunk of code, its take it all or nothing.
>
>
> (3)
> I asked for documentation on how a user is supposed to interact with
> this change. This should help define what a parser is, how a user creates
> a parser, does an analyst have to run Maven just to deploy a parser?
>
>
>
> (4)
> I asked for a complete test plan on how to test all of this
> functionality.
>
>
> I don't think I received for very good responses from you on these
> questions. Comments from you like this [1], do not help me review the code.
>
> Since this PR is not adding new functionality the testing is the usual
>> smoke test for parser functionality per functional area ( Full Dev has
>> data, Metron UI sees the configurations ). The existing parsers should
>> work, you should not notice a difference.
>
>
>
>> I feel this PR delivers the things discussed on list, and as described in
>> the status updates
>
>
> Clearly this amount of code does not result in no net-new functionality.
> A usual smoke test is just not good enough here.
> You can't refer to requirements that have been made on the mailing list.
> That is not sufficient.
> But m
> aybe th
> is
> has
> all
> changed since my first review attempt. If so, please point me
> to where this is documented
> and I will take a look.
>
> You can see
>
> a symptom of
> these problems in
> the discuss thread that you opened yesterday. We don't have agreement in
> the community on basic definitions like what a parser is. That's not a
> good sign.
>
>
> I hope this helps you understand (at least my view point of) some of the
> problems in reviewing such a large PR. Even if we break this up into
> smaller PRs, we still need the same things. What is the scope? What does
> this touch? How is a user going to use this? How do I test this? It is
> just much easier to answer these questions on a smaller PR.
>
> I provide this feedback with the highest level of respect for what you've
> been able to accomplish. I just want to provide candid feedback to perhaps
> help push this over the finish line.
>
>
> [1] https://github.com/apache/metron/pull/530#issuecomment-306604503
>
>
>
>
>
>
>
> On Wed, Sep 20, 2017 at 6:04 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> I am not taking any of this as starting to do a flame war :) And your
>> concern is about review not merging right? If it is all reviewed and
>> accepted, why would 6 merges be preferable? That doesn’t make sense.
>> Breaking it up only makes sense with regards to review, so that is how
>> I’ll take it.
>>
>> And I *am* all ears for how to break up the code. Like literally HOW.
>> Like : ‘I would diff a folder with master, create a new branch for the pr
>> and patch just that’.
>> Just 'break up the massive working thing into smaller chucks' is not
>> enough for my relative experience with git and github to you Nick.
>>
>> So if all we want are code review chunks, regardless of being able to run
>> them and see them work then ( but also not break the regular build and
>> full_dev ):
>>
>> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
>> code review only, doesn’t do anything
>> 2. PR for Archetype ( just building the result, unit integration tests )
>> code review and generate project review. projects built by archetype :
>> => not buildable not installable not testable
>> relies on changes to parser testing etc. to work with non-hard coded
>> paths for test resources etc etc
>> 3. PR with Parsers copied to extensions
>> code review only. not buildable not installable not testable
>> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move
>> to new parser structure, load install, grok support etc etc.
>> regression testable, status quo ante functionality.
>> still huge
>> will still be a problem
>> cannot be broken down and built / pass travis at this point
>> this is the integration
>> 5. PR of Rest
>> can install and uninstall via rest
>> 6. PR of UI
>> can list, install uninstall via UI
>>
>> I did not think then that we wanted PRs that don’t build or test or run
>> the changes they have ( but the branch builds and passes travis ), but just
>> have code to review.
>>
>> But, as you said, it has been since April, and only Bundles has been
>> reviewed. If nobody but Matt is even going to _attempt_ code review, then
>> it may now be implicitly required that I do this.
>>
>>
>> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>>
>> I was just responding to your statement "Well, if there is an
>> alternative merge strategy, I’m all ears." I understand that you disagree,
>> but this is an alternative strategy. I think this approach is feasible and
>> I outlined how here [1].
>>
>> The criticism of this approach is the amount of effort and time to do
>> this, which I agree with. It will take a good deal of effort.
>> I acquiesced to this criticism at the time because I thought we were trying
>> to push this change in quickly. But it has been a couple months now and we
>> still have a bunch of code to merge.
>>
>> This is no criticism of you or anyone here. What Otto has been doing in
>> keeping this branch merged with master is god damn heroic. And I am so
>> excited to see this functionality hit master. I just wanted to brainstorm
>> on ideas to help get this in. Right now, I am unsure of the path and
>> timeline on how we are going to get this merged.
>>
>> Please read this in the kindest possible light. I am not trying to start
>> a flame war or offend anyone.
>>
>>
>> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>>
>>
>>
>> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>>> that from an already integrated and working branch.
>>> Do you mean I…
>>> - create patch files of whole directories and do a pr per directory, but
>>> the build doesn’t work?
>>>
>>>
>>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>>
>>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>>
>>> > Nick: Are you going to bite-off small chunks of the feature branch
>>> and introduce PRs against master?
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> Well, if there is an alternative merge strategy, I’m all ears.
>>>> As it stands, the only complete code / conceptual review has been the
>>>> bundles-lib by mattf.
>>>> So there is still a great deal of review activity to do.
>>>>
>>>> Also conceptually there is ( might not be complete )
>>>>
>>>> * the discuss thread topic from this morning ( metron parsers v.
>>>> extensions wrt registration and management )
>>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>>
>>>>
>>>>
>>>>
>>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> Ok, so it sounds like you are envisioning a "big bang" merge between
>>>> the feature branch and master at some point. Not ideal in my mind, but
>>>> maybe we need to be more pragmatic with this one.
>>>>
>>>> Do we have other tasks (like these PRs) to complete before we are ready
>>>> to consider the merge?
>>>>
>>>> I am just looking to help however I can in killing this feature branch;
>>>> meaning I want your code in master, as soon as possible. :)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>>> against master ( such as hdfs service ability to set permissions ).
>>>>> I don’t think we have talked about the end game PR from feature to
>>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>>> agreed on.
>>>>>
>>>>>
>>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> So it sounds like these PRs do move us closer to bringing the two
>>>>> branches together. But I think I am missing your high-level approach
>>>>> though.
>>>>>
>>>>> How are we going to get all of the functionality from the feature
>>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>>> branch and introduce PRs against master?
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>>> BundleSystem changes.
>>>>>> I would rather bring them down to feature before I do master
>>>>>> integration, than integrate master
>>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>>> that.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>>> wrote:
>>>>>>
>>>>>> Hi Otto -
>>>>>>
>>>>>> What is the plan for bringing the feature branch and master together?
>>>>>> Do
>>>>>> these PRs move us closer to bringing the two branches together?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>>> > https://github.com/apache/metron/pull/761?
>>>>>> >
>>>>>> > The next time I take master is going to be a bit of integration
>>>>>> work, and I
>>>>>> > would like to unstack and get my stuff integrated first.
>>>>>> >
>>>>>>
>>>>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
Nick, thanks for taking the time to reply, I have no doubts about your
motivations and intent. Hopefully we get through this point by point
stuff, which is always difficult on email.
https://cwiki.apache.org/confluence/display/METRON/Metron+Extension+System+and+Parser+Extensions
This is my recent attempt at creating a framework for the review. It
includes testing areas, review areas etc. I have not received any feedback
on it so I have to assume that
you have not seen it. My stated hope is that through questions about the
confluence I can address anything missing to make things reviewable, or
more reviewable. If you have reviewed it, and are just not referring to it,
I apologize.
Please take a look, and provide feedback so that I can address it there if
you have not.
As far as the problem being lack of community commitment, that is not what
I was trying to imply. I was, in my mind agreeing with you that the size
of the changes has proven to be a barrier in practice, despite our
agreement in 777 to proceed as is, and our discussion about the feature
branch.
As far as (1) the scope, I tried to lay out the areas of change and what
was changed in the description of 777. I also re-listed separately the
specific files changed, and what functional area the change referred to in
the PR. Can you point me to an example of a different definition of scope
that I can copy? Is it a more descriptive but still high level statement
that would help?
As far as (2) I have outlined the parts that could be considered separately
( bundles, archetype, maven plugin ). Once we switch to the new parser
structure, there is no way to cut it down and still have a working build,
because the new parser structure and loading touches pretty much all
levels. This is not that different from what we discussed at the time. I
do not know what is in there that should not be in there, aside from the 3
areas I mentioned that could be reviewed as not integrated components. Are
you referring to those areas or something else?
As far as (3) - In the original 777, there was no user facing changes.
Only developer changes, which I addressed in the adding system parsers
guide based on Mike’s feedback. Maybe we are using different terminology
and just missing each other. Based on the original 777, a user ( someone
who deploys metron and uses the ambari configuration and the management ui,
pushes new parsers to zookeeper, uses kibana etc) would not see any
changes, or should not have seen any changes. There was no new
functionality to test from a user POV. Mike did find a regression in the
config ui wrt grok rules editing, which I addressed, but that was not new
function problem, but a regression. The user facing changes where in 942 (
rest + stellar management command) and where documented, and are also now
in the UI pr. Asking how a user creates a parser in 777 when I
specifically put that in 942 to cut down on scope, while also questioning
the scope is confusing to me.
As far as (4) - please see the confluence pages.
What I would like to do, and what it seems to me that we need to do ( again
I apologize if I am wrong ), is to catch up on the confluence and where
things currently stand, and then move the discussion on from there. I feel
like a lot has gone on in 777 and there that relate to your concerns (
although I am not saying they address them ).
On September 21, 2017 at 11:58:51, Nick Allen (nick@nickallen.org) wrote:
But, as you said, it has been since April, and only Bundles has been
> reviewed. If nobody but Matt is even going to _attempt_ code review, then
> it may now be implicitly required that I do this.
I disagree
that the problem is lack of community commitment to the change
. We have had quite a few core team members who at least began a review. I
can't speak for anyone else, but I'll speak for my own experience
in attempting to review 777.
These are the reasons why I was not able to close out my review of 777
with a +1.
(1)
T
he scope of this change is largely undefined.
I do
not
have
a good understanding
of what this thing actually does
and does not do
. What does it touch and what does it not touch?
(2)
I feel like there are lots of different pieces of functionality
in the FB
that we've tied all together that doesn't necessarily need to be.
With this being one big
chunk of code, its take it all or nothing.
(3)
I asked for documentation on how a user is supposed to interact with this
change. This should help define what a parser is, how a user creates a
parser, does an analyst have to run Maven just to deploy a parser?
(4)
I asked for a complete test plan on how to test all of this functionality.
I don't think I received for very good responses from you on these
questions. Comments from you like this [1], do not help me review the code.
Since this PR is not adding new functionality the testing is the usual
> smoke test for parser functionality per functional area ( Full Dev has
> data, Metron UI sees the configurations ). The existing parsers should
> work, you should not notice a difference.
> I feel this PR delivers the things discussed on list, and as described in
> the status updates
Clearly this amount of code does not result in no net-new functionality. A
usual smoke test is just not good enough here.
You can't refer to requirements that have been made on the mailing list.
That is not sufficient.
But m
aybe th
is
has
all
changed since my first review attempt. If so, please point me
to where this is documented
and I will take a look.
You can see
a symptom of
these problems in
the discuss thread that you opened yesterday. We don't have agreement in
the community on basic definitions like what a parser is. That's not a
good sign.
I hope this helps you understand (at least my view point of) some of the
problems in reviewing such a large PR. Even if we break this up into
smaller PRs, we still need the same things. What is the scope? What does
this touch? How is a user going to use this? How do I test this? It is
just much easier to answer these questions on a smaller PR.
I provide this feedback with the highest level of respect for what you've
been able to accomplish. I just want to provide candid feedback to perhaps
help push this over the finish line.
[1] https://github.com/apache/metron/pull/530#issuecomment-306604503
On Wed, Sep 20, 2017 at 6:04 PM Otto Fowler <ot...@gmail.com> wrote:
> I am not taking any of this as starting to do a flame war :) And your
> concern is about review not merging right? If it is all reviewed and
> accepted, why would 6 merges be preferable? That doesn’t make sense.
> Breaking it up only makes sense with regards to review, so that is how
> I’ll take it.
>
> And I *am* all ears for how to break up the code. Like literally HOW.
> Like : ‘I would diff a folder with master, create a new branch for the pr
> and patch just that’.
> Just 'break up the massive working thing into smaller chucks' is not
> enough for my relative experience with git and github to you Nick.
>
> So if all we want are code review chunks, regardless of being able to run
> them and see them work then ( but also not break the regular build and
> full_dev ):
>
> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
> code review only, doesn’t do anything
> 2. PR for Archetype ( just building the result, unit integration tests )
> code review and generate project review. projects built by archetype :
> => not buildable not installable not testable
> relies on changes to parser testing etc. to work with non-hard coded paths
> for test resources etc etc
> 3. PR with Parsers copied to extensions
> code review only. not buildable not installable not testable
> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move to
> new parser structure, load install, grok support etc etc.
> regression testable, status quo ante functionality.
> still huge
> will still be a problem
> cannot be broken down and built / pass travis at this point
> this is the integration
> 5. PR of Rest
> can install and uninstall via rest
> 6. PR of UI
> can list, install uninstall via UI
>
> I did not think then that we wanted PRs that don’t build or test or run
> the changes they have ( but the branch builds and passes travis ), but just
> have code to review.
>
> But, as you said, it has been since April, and only Bundles has been
> reviewed. If nobody but Matt is even going to _attempt_ code review, then
> it may now be implicitly required that I do this.
>
>
> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>
> I was just responding to your statement "Well, if there is an alternative
> merge strategy, I’m all ears." I understand that you disagree, but this is
> an alternative strategy. I think this approach is feasible and I outlined
> how here [1].
>
> The criticism of this approach is the amount of effort and time to do
> this, which I agree with. It will take a good deal of effort.
> I acquiesced to this criticism at the time because I thought we were trying
> to push this change in quickly. But it has been a couple months now and we
> still have a bunch of code to merge.
>
> This is no criticism of you or anyone here. What Otto has been doing in
> keeping this branch merged with master is god damn heroic. And I am so
> excited to see this functionality hit master. I just wanted to brainstorm
> on ideas to help get this in. Right now, I am unsure of the path and
> timeline on how we are going to get this merged.
>
> Please read this in the kindest possible light. I am not trying to start
> a flame war or offend anyone.
>
>
> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>
>
>
> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>> that from an already integrated and working branch.
>> Do you mean I…
>> - create patch files of whole directories and do a pr per directory, but
>> the build doesn’t work?
>>
>>
>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
>>
>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>
>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>
>> > Nick: Are you going to bite-off small chunks of the feature branch and
>> introduce PRs against master?
>>
>>
>>
>>
>>
>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> Well, if there is an alternative merge strategy, I’m all ears.
>>> As it stands, the only complete code / conceptual review has been the
>>> bundles-lib by mattf.
>>> So there is still a great deal of review activity to do.
>>>
>>> Also conceptually there is ( might not be complete )
>>>
>>> * the discuss thread topic from this morning ( metron parsers v.
>>> extensions wrt registration and management )
>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>
>>>
>>>
>>>
>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> Ok, so it sounds like you are envisioning a "big bang" merge between the
>>> feature branch and master at some point. Not ideal in my mind, but maybe
>>> we need to be more pragmatic with this one.
>>>
>>> Do we have other tasks (like these PRs) to complete before we are ready
>>> to consider the merge?
>>>
>>> I am just looking to help however I can in killing this feature branch;
>>> meaning I want your code in master, as soon as possible. :)
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>> against master ( such as hdfs service ability to set permissions ).
>>>> I don’t think we have talked about the end game PR from feature to
>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>> agreed on.
>>>>
>>>>
>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> So it sounds like these PRs do move us closer to bringing the two
>>>> branches together. But I think I am missing your high-level approach
>>>> though.
>>>>
>>>> How are we going to get all of the functionality from the feature
>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>> branch and introduce PRs against master?
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>> BundleSystem changes.
>>>>> I would rather bring them down to feature before I do master
>>>>> integration, than integrate master
>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>> that.
>>>>>
>>>>>
>>>>>
>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> Hi Otto -
>>>>>
>>>>> What is the plan for bringing the feature branch and master together?
>>>>> Do
>>>>> these PRs move us closer to bringing the two branches together?
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>> > https://github.com/apache/metron/pull/761?
>>>>> >
>>>>> > The next time I take master is going to be a bit of integration
>>>>> work, and I
>>>>> > would like to unstack and get my stuff integrated first.
>>>>> >
>>>>>
>>>>>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
>
> But, as you said, it has been since April, and only Bundles has been
> reviewed. If nobody but Matt is even going to _attempt_ code review, then
> it may now be implicitly required that I do this.
I disagree
that the problem is lack of community commitment to the change
. We have had quite a few core team members who at least began a review. I
can't speak for anyone else, but I'll speak for my own experience
in attempting to review 777.
These are the reasons why I was not able to close out my review of 777
with a +1.
(1)
T
he scope of this change is largely undefined.
I do
not
have
a good understanding
of what this thing actually does
and does not do
. What does it touch and what does it not touch?
(2)
I feel like there are lots of different pieces of functionality
in the FB
that we've tied all together that doesn't necessarily need to be.
With this being one big
chunk of code, its take it all or nothing.
(3)
I asked for documentation on how a user is supposed to interact with this
change. This should help define what a parser is, how a user creates a
parser, does an analyst have to run Maven just to deploy a parser?
(4)
I asked for a complete test plan on how to test all of this functionality.
I don't think I received for very good responses from you on these
questions. Comments from you like this [1], do not help me review the code.
Since this PR is not adding new functionality the testing is the usual
> smoke test for parser functionality per functional area ( Full Dev has
> data, Metron UI sees the configurations ). The existing parsers should
> work, you should not notice a difference.
> I feel this PR delivers the things discussed on list, and as described in
> the status updates
Clearly this amount of code does not result in no net-new functionality. A
usual smoke test is just not good enough here.
You can't refer to requirements that have been made on the mailing list.
That is not sufficient.
But m
aybe th
is
has
all
changed since my first review attempt. If so, please point me
to where this is documented
and I will take a look.
You can see
a symptom of
these problems in
the discuss thread that you opened yesterday. We don't have agreement in
the community on basic definitions like what a parser is. That's not a
good sign.
I hope this helps you understand (at least my view point of) some of the
problems in reviewing such a large PR. Even if we break this up into
smaller PRs, we still need the same things. What is the scope? What does
this touch? How is a user going to use this? How do I test this? It is
just much easier to answer these questions on a smaller PR.
I provide this feedback with the highest level of respect for what you've
been able to accomplish. I just want to provide candid feedback to perhaps
help push this over the finish line.
[1] https://github.com/apache/metron/pull/530#issuecomment-306604503
On Wed, Sep 20, 2017 at 6:04 PM Otto Fowler <ot...@gmail.com> wrote:
> I am not taking any of this as starting to do a flame war :) And your
> concern is about review not merging right? If it is all reviewed and
> accepted, why would 6 merges be preferable? That doesn’t make sense.
> Breaking it up only makes sense with regards to review, so that is how
> I’ll take it.
>
> And I *am* all ears for how to break up the code. Like literally HOW.
> Like : ‘I would diff a folder with master, create a new branch for the pr
> and patch just that’.
> Just 'break up the massive working thing into smaller chucks' is not
> enough for my relative experience with git and github to you Nick.
>
> So if all we want are code review chunks, regardless of being able to run
> them and see them work then ( but also not break the regular build and
> full_dev ):
>
> 1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
> code review only, doesn’t do anything
> 2. PR for Archetype ( just building the result, unit integration tests )
> code review and generate project review. projects built by archetype :
> => not buildable not installable not testable
> relies on changes to parser testing etc. to work with non-hard coded paths
> for test resources etc etc
> 3. PR with Parsers copied to extensions
> code review only. not buildable not installable not testable
> 4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move to
> new parser structure, load install, grok support etc etc.
> regression testable, status quo ante functionality.
> still huge
> will still be a problem
> cannot be broken down and built / pass travis at this point
> this is the integration
> 5. PR of Rest
> can install and uninstall via rest
> 6. PR of UI
> can list, install uninstall via UI
>
> I did not think then that we wanted PRs that don’t build or test or run
> the changes they have ( but the branch builds and passes travis ), but just
> have code to review.
>
> But, as you said, it has been since April, and only Bundles has been
> reviewed. If nobody but Matt is even going to _attempt_ code review, then
> it may now be implicitly required that I do this.
>
>
> On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
>
> I was just responding to your statement "Well, if there is an alternative
> merge strategy, I’m all ears." I understand that you disagree, but this is
> an alternative strategy. I think this approach is feasible and I outlined
> how here [1].
>
> The criticism of this approach is the amount of effort and time to do
> this, which I agree with. It will take a good deal of effort.
> I acquiesced to this criticism at the time because I thought we were trying
> to push this change in quickly. But it has been a couple months now and we
> still have a bunch of code to merge.
>
> This is no criticism of you or anyone here. What Otto has been doing in
> keeping this branch merged with master is god damn heroic. And I am so
> excited to see this functionality hit master. I just wanted to brainstorm
> on ideas to help get this in. Right now, I am unsure of the path and
> timeline on how we are going to get this merged.
>
> Please read this in the kindest possible light. I am not trying to start
> a flame war or offend anyone.
>
>
> [1] https://github.com/apache/metron/pull/530#issuecomment-306806217
>
>
>
> On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> “Bite off small chunks” is what I keep hearing. I have no idea how to do
>> that from an already integrated and working branch.
>> Do you mean I…
>> - create patch files of whole directories and do a pr per directory, but
>> the build doesn’t work?
>>
>>
>> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
>>
>> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>>
>> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>>
>> > Nick: Are you going to bite-off small chunks of the feature branch and
>> introduce PRs against master?
>>
>>
>>
>>
>>
>> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> Well, if there is an alternative merge strategy, I’m all ears.
>>> As it stands, the only complete code / conceptual review has been the
>>> bundles-lib by mattf.
>>> So there is still a great deal of review activity to do.
>>>
>>> Also conceptually there is ( might not be complete )
>>>
>>> * the discuss thread topic from this morning ( metron parsers v.
>>> extensions wrt registration and management )
>>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>>
>>>
>>>
>>>
>>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> Ok, so it sounds like you are envisioning a "big bang" merge between the
>>> feature branch and master at some point. Not ideal in my mind, but maybe
>>> we need to be more pragmatic with this one.
>>>
>>> Do we have other tasks (like these PRs) to complete before we are ready
>>> to consider the merge?
>>>
>>> I am just looking to help however I can in killing this feature branch;
>>> meaning I want your code in master, as soon as possible. :)
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> So, were the functionality is not FB related, I have been doing PR’s
>>>> against master ( such as hdfs service ability to set permissions ).
>>>> I don’t think we have talked about the end game PR from feature to
>>>> master, I am don’t know how we would do multiple PRs to bring it down
>>>> once ‘accepted’. I think that approach needs to to be discussed and
>>>> agreed on.
>>>>
>>>>
>>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> So it sounds like these PRs do move us closer to bringing the two
>>>> branches together. But I think I am missing your high-level approach
>>>> though.
>>>>
>>>> How are we going to get all of the functionality from the feature
>>>> branch into master? Are you going to bite-off small chunks of the feature
>>>> branch and introduce PRs against master?
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>>> BundleSystem changes.
>>>>> I would rather bring them down to feature before I do master
>>>>> integration, than integrate master
>>>>> into feature and then bring it out to the stacked PR’s. Simple as
>>>>> that.
>>>>>
>>>>>
>>>>>
>>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>>> wrote:
>>>>>
>>>>> Hi Otto -
>>>>>
>>>>> What is the plan for bringing the feature branch and master together?
>>>>> Do
>>>>> these PRs move us closer to bringing the two branches together?
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>>> > https://github.com/apache/metron/pull/761?
>>>>> >
>>>>> > The next time I take master is going to be a bit of integration
>>>>> work, and I
>>>>> > would like to unstack and get my stuff integrated first.
>>>>> >
>>>>>
>>>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
I am not taking any of this as starting to do a flame war :) And your
concern is about review not merging right? If it is all reviewed and
accepted, why would 6 merges be preferable? That doesn’t make sense.
Breaking it up only makes sense with regards to review, so that is how I’ll
take it.
And I *am* all ears for how to break up the code. Like literally HOW.
Like : ‘I would diff a folder with master, create a new branch for the pr
and patch just that’.
Just 'break up the massive working thing into smaller chucks' is not enough
for my relative experience with git and github to you Nick.
So if all we want are code review chunks, regardless of being able to run
them and see them work then ( but also not break the regular build and
full_dev ):
1. PR for Bundle-Lib And Maven Plugin ( but that is already reviewed )
code review only, doesn’t do anything
2. PR for Archetype ( just building the result, unit integration tests )
code review and generate project review. projects built by archetype : =>
not buildable not installable not testable
relies on changes to parser testing etc. to work with non-hard coded paths
for test resources etc etc
3. PR with Parsers copied to extensions
code review only. not buildable not installable not testable
4. PR with the rest of 777 + grok fixes ( all non ui non rest ) -> move to
new parser structure, load install, grok support etc etc.
regression testable, status quo ante functionality.
still huge
will still be a problem
cannot be broken down and built / pass travis at this point
this is the integration
5. PR of Rest
can install and uninstall via rest
6. PR of UI
can list, install uninstall via UI
I did not think then that we wanted PRs that don’t build or test or run the
changes they have ( but the branch builds and passes travis ), but just
have code to review.
But, as you said, it has been since April, and only Bundles has been
reviewed. If nobody but Matt is even going to _attempt_ code review, then
it may now be implicitly required that I do this.
On September 20, 2017 at 16:41:46, Nick Allen (nick@nickallen.org) wrote:
I was just responding to your statement "Well, if there is an alternative
merge strategy, I’m all ears." I understand that you disagree, but this is
an alternative strategy. I think this approach is feasible and I outlined
how here [1].
The criticism of this approach is the amount of effort and time to do this,
which I agree with. It will take a good deal of effort. I acquiesced to
this criticism at the time because I thought we were trying to push this
change in quickly. But it has been a couple months now and we still have a
bunch of code to merge.
This is no criticism of you or anyone here. What Otto has been doing in
keeping this branch merged with master is god damn heroic. And I am so
excited to see this functionality hit master. I just wanted to brainstorm
on ideas to help get this in. Right now, I am unsure of the path and
timeline on how we are going to get this merged.
Please read this in the kindest possible light. I am not trying to start a
flame war or offend anyone.
[1] https://github.com/apache/metron/pull/530#issuecomment-306806217
On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com> wrote:
> “Bite off small chunks” is what I keep hearing. I have no idea how to do
> that from an already integrated and working branch.
> Do you mean I…
> - create patch files of whole directories and do a pr per directory, but
> the build doesn’t work?
>
>
> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
>
> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>
> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>
> > Nick: Are you going to bite-off small chunks of the feature branch and
> introduce PRs against master?
>
>
>
>
>
> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> Well, if there is an alternative merge strategy, I’m all ears.
>> As it stands, the only complete code / conceptual review has been the
>> bundles-lib by mattf.
>> So there is still a great deal of review activity to do.
>>
>> Also conceptually there is ( might not be complete )
>>
>> * the discuss thread topic from this morning ( metron parsers v.
>> extensions wrt registration and management )
>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>
>>
>>
>>
>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org) wrote:
>>
>> Ok, so it sounds like you are envisioning a "big bang" merge between the
>> feature branch and master at some point. Not ideal in my mind, but maybe
>> we need to be more pragmatic with this one.
>>
>> Do we have other tasks (like these PRs) to complete before we are ready
>> to consider the merge?
>>
>> I am just looking to help however I can in killing this feature branch;
>> meaning I want your code in master, as soon as possible. :)
>>
>>
>>
>>
>>
>>
>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> So, were the functionality is not FB related, I have been doing PR’s
>>> against master ( such as hdfs service ability to set permissions ).
>>> I don’t think we have talked about the end game PR from feature to
>>> master, I am don’t know how we would do multiple PRs to bring it down
>>> once ‘accepted’. I think that approach needs to to be discussed and
>>> agreed on.
>>>
>>>
>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> So it sounds like these PRs do move us closer to bringing the two
>>> branches together. But I think I am missing your high-level approach
>>> though.
>>>
>>> How are we going to get all of the functionality from the feature branch
>>> into master? Are you going to bite-off small chunks of the feature branch
>>> and introduce PRs against master?
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>> BundleSystem changes.
>>>> I would rather bring them down to feature before I do master
>>>> integration, than integrate master
>>>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>>>
>>>>
>>>>
>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> Hi Otto -
>>>>
>>>> What is the plan for bringing the feature branch and master together? Do
>>>> these PRs move us closer to bringing the two branches together?
>>>>
>>>> Thanks
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>> > https://github.com/apache/metron/pull/761?
>>>> >
>>>> > The next time I take master is going to be a bit of integration work,
>>>> and I
>>>> > would like to unstack and get my stuff integrated first.
>>>> >
>>>>
>>>>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
I was just responding to your statement "Well, if there is an alternative
merge strategy, I’m all ears." I understand that you disagree, but this is
an alternative strategy. I think this approach is feasible and I outlined
how here [1].
The criticism of this approach is the amount of effort and time to do this,
which I agree with. It will take a good deal of effort. I acquiesced to
this criticism at the time because I thought we were trying to push this
change in quickly. But it has been a couple months now and we still have a
bunch of code to merge.
This is no criticism of you or anyone here. What Otto has been doing in
keeping this branch merged with master is god damn heroic. And I am so
excited to see this functionality hit master. I just wanted to brainstorm
on ideas to help get this in. Right now, I am unsure of the path and
timeline on how we are going to get this merged.
Please read this in the kindest possible light. I am not trying to start a
flame war or offend anyone.
[1] https://github.com/apache/metron/pull/530#issuecomment-306806217
On Wed, Sep 20, 2017 at 3:37 PM Otto Fowler <ot...@gmail.com> wrote:
> “Bite off small chunks” is what I keep hearing. I have no idea how to do
> that from an already integrated and working branch.
> Do you mean I…
> - create patch files of whole directories and do a pr per directory, but
> the build doesn’t work?
>
>
> On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
>
> > Otto: Well, if there is an alternative merge strategy, I’m all ears.
>
> Yes, the alternative strategy is what I mentioned. :) Copied in below.
>
> > Nick: Are you going to bite-off small chunks of the feature branch and
> introduce PRs against master?
>
>
>
>
>
> On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> Well, if there is an alternative merge strategy, I’m all ears.
>> As it stands, the only complete code / conceptual review has been the
>> bundles-lib by mattf.
>> So there is still a great deal of review activity to do.
>>
>> Also conceptually there is ( might not be complete )
>>
>> * the discuss thread topic from this morning ( metron parsers v.
>> extensions wrt registration and management )
>> * default configurations ( parser, enrichment, indexing, elasticsearch)
>>
>>
>>
>>
>> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org) wrote:
>>
>> Ok, so it sounds like you are envisioning a "big bang" merge between the
>> feature branch and master at some point. Not ideal in my mind, but maybe
>> we need to be more pragmatic with this one.
>>
>> Do we have other tasks (like these PRs) to complete before we are ready
>> to consider the merge?
>>
>> I am just looking to help however I can in killing this feature branch;
>> meaning I want your code in master, as soon as possible. :)
>>
>>
>>
>>
>>
>>
>> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> So, were the functionality is not FB related, I have been doing PR’s
>>> against master ( such as hdfs service ability to set permissions ).
>>> I don’t think we have talked about the end game PR from feature to
>>> master, I am don’t know how we would do multiple PRs to bring it down
>>> once ‘accepted’. I think that approach needs to to be discussed and
>>> agreed on.
>>>
>>>
>>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> So it sounds like these PRs do move us closer to bringing the two
>>> branches together. But I think I am missing your high-level approach
>>> though.
>>>
>>> How are we going to get all of the functionality from the feature branch
>>> into master? Are you going to bite-off small chunks of the feature branch
>>> and introduce PRs against master?
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>>> Right now, these two PR’s are stacked, the UI depends on the
>>>> BundleSystem changes.
>>>> I would rather bring them down to feature before I do master
>>>> integration, than integrate master
>>>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>>>
>>>>
>>>>
>>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>>> wrote:
>>>>
>>>> Hi Otto -
>>>>
>>>> What is the plan for bringing the feature branch and master together? Do
>>>> these PRs move us closer to bringing the two branches together?
>>>>
>>>> Thanks
>>>>
>>>>
>>>>
>>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>>> wrote:
>>>>
>>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>>> > https://github.com/apache/metron/pull/761?
>>>> >
>>>> > The next time I take master is going to be a bit of integration work,
>>>> and I
>>>> > would like to unstack and get my stuff integrated first.
>>>> >
>>>>
>>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
“Bite off small chunks” is what I keep hearing. I have no idea how to do
that from an already integrated and working branch.
Do you mean I…
- create patch files of whole directories and do a pr per directory, but
the build doesn’t work?
On September 20, 2017 at 15:07:56, Nick Allen (nick@nickallen.org) wrote:
> Otto: Well, if there is an alternative merge strategy, I’m all ears.
Yes, the alternative strategy is what I mentioned. :) Copied in below.
> Nick: Are you going to bite-off small chunks of the feature branch and
introduce PRs against master?
On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
wrote:
> Well, if there is an alternative merge strategy, I’m all ears.
> As it stands, the only complete code / conceptual review has been the
> bundles-lib by mattf.
> So there is still a great deal of review activity to do.
>
> Also conceptually there is ( might not be complete )
>
> * the discuss thread topic from this morning ( metron parsers v.
> extensions wrt registration and management )
> * default configurations ( parser, enrichment, indexing, elasticsearch)
>
>
>
>
> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org) wrote:
>
> Ok, so it sounds like you are envisioning a "big bang" merge between the
> feature branch and master at some point. Not ideal in my mind, but maybe
> we need to be more pragmatic with this one.
>
> Do we have other tasks (like these PRs) to complete before we are ready to
> consider the merge?
>
> I am just looking to help however I can in killing this feature branch;
> meaning I want your code in master, as soon as possible. :)
>
>
>
>
>
>
> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> So, were the functionality is not FB related, I have been doing PR’s
>> against master ( such as hdfs service ability to set permissions ).
>> I don’t think we have talked about the end game PR from feature to
>> master, I am don’t know how we would do multiple PRs to bring it down
>> once ‘accepted’. I think that approach needs to to be discussed and
>> agreed on.
>>
>>
>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org) wrote:
>>
>> So it sounds like these PRs do move us closer to bringing the two
>> branches together. But I think I am missing your high-level approach
>> though.
>>
>> How are we going to get all of the functionality from the feature branch
>> into master? Are you going to bite-off small chunks of the feature branch
>> and introduce PRs against master?
>>
>>
>>
>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> Right now, these two PR’s are stacked, the UI depends on the
>>> BundleSystem changes.
>>> I would rather bring them down to feature before I do master
>>> integration, than integrate master
>>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>>
>>>
>>>
>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> Hi Otto -
>>>
>>> What is the plan for bringing the feature branch and master together? Do
>>> these PRs move us closer to bringing the two branches together?
>>>
>>> Thanks
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>> > https://github.com/apache/metron/pull/761?
>>> >
>>> > The next time I take master is going to be a bit of integration work,
>>> and I
>>> > would like to unstack and get my stuff integrated first.
>>> >
>>>
>>>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
> Otto: Well, if there is an alternative merge strategy, I’m all ears.
Yes, the alternative strategy is what I mentioned. :) Copied in below.
> Nick: Are you going to bite-off small chunks of the feature branch and
introduce PRs against master?
On Wed, Sep 20, 2017 at 12:02 PM Otto Fowler <ot...@gmail.com>
wrote:
> Well, if there is an alternative merge strategy, I’m all ears.
> As it stands, the only complete code / conceptual review has been the
> bundles-lib by mattf.
> So there is still a great deal of review activity to do.
>
> Also conceptually there is ( might not be complete )
>
> * the discuss thread topic from this morning ( metron parsers v.
> extensions wrt registration and management )
> * default configurations ( parser, enrichment, indexing, elasticsearch)
>
>
>
>
> On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org) wrote:
>
> Ok, so it sounds like you are envisioning a "big bang" merge between the
> feature branch and master at some point. Not ideal in my mind, but maybe
> we need to be more pragmatic with this one.
>
> Do we have other tasks (like these PRs) to complete before we are ready to
> consider the merge?
>
> I am just looking to help however I can in killing this feature branch;
> meaning I want your code in master, as soon as possible. :)
>
>
>
>
>
>
> On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> So, were the functionality is not FB related, I have been doing PR’s
>> against master ( such as hdfs service ability to set permissions ).
>> I don’t think we have talked about the end game PR from feature to
>> master, I am don’t know how we would do multiple PRs to bring it down
>> once ‘accepted’. I think that approach needs to to be discussed and
>> agreed on.
>>
>>
>> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org) wrote:
>>
>> So it sounds like these PRs do move us closer to bringing the two
>> branches together. But I think I am missing your high-level approach
>> though.
>>
>> How are we going to get all of the functionality from the feature branch
>> into master? Are you going to bite-off small chunks of the feature branch
>> and introduce PRs against master?
>>
>>
>>
>> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> Right now, these two PR’s are stacked, the UI depends on the
>>> BundleSystem changes.
>>> I would rather bring them down to feature before I do master
>>> integration, than integrate master
>>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>>
>>>
>>>
>>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org)
>>> wrote:
>>>
>>> Hi Otto -
>>>
>>> What is the plan for bringing the feature branch and master together? Do
>>> these PRs move us closer to bringing the two branches together?
>>>
>>> Thanks
>>>
>>>
>>>
>>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>>> wrote:
>>>
>>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>>> > https://github.com/apache/metron/pull/761?
>>> >
>>> > The next time I take master is going to be a bit of integration work,
>>> and I
>>> > would like to unstack and get my stuff integrated first.
>>> >
>>>
>>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
Well, if there is an alternative merge strategy, I’m all ears.
As it stands, the only complete code / conceptual review has been the
bundles-lib by mattf.
So there is still a great deal of review activity to do.
Also conceptually there is ( might not be complete )
* the discuss thread topic from this morning ( metron parsers v. extensions
wrt registration and management )
* default configurations ( parser, enrichment, indexing, elasticsearch)
On September 20, 2017 at 11:37:17, Nick Allen (nick@nickallen.org) wrote:
Ok, so it sounds like you are envisioning a "big bang" merge between the
feature branch and master at some point. Not ideal in my mind, but maybe
we need to be more pragmatic with this one.
Do we have other tasks (like these PRs) to complete before we are ready to
consider the merge?
I am just looking to help however I can in killing this feature branch;
meaning I want your code in master, as soon as possible. :)
On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
wrote:
> So, were the functionality is not FB related, I have been doing PR’s
> against master ( such as hdfs service ability to set permissions ).
> I don’t think we have talked about the end game PR from feature to master,
> I am don’t know how we would do multiple PRs to bring it down
> once ‘accepted’. I think that approach needs to to be discussed and
> agreed on.
>
>
> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org) wrote:
>
> So it sounds like these PRs do move us closer to bringing the two branches
> together. But I think I am missing your high-level approach though.
>
> How are we going to get all of the functionality from the feature branch
> into master? Are you going to bite-off small chunks of the feature branch
> and introduce PRs against master?
>
>
>
> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> Right now, these two PR’s are stacked, the UI depends on the BundleSystem
>> changes.
>> I would rather bring them down to feature before I do master integration,
>> than integrate master
>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>
>>
>>
>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org) wrote:
>>
>> Hi Otto -
>>
>> What is the plan for bringing the feature branch and master together? Do
>> these PRs move us closer to bringing the two branches together?
>>
>> Thanks
>>
>>
>>
>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>> > https://github.com/apache/metron/pull/761?
>> >
>> > The next time I take master is going to be a bit of integration work,
>> and I
>> > would like to unstack and get my stuff integrated first.
>> >
>>
>>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
Ok, so it sounds like you are envisioning a "big bang" merge between the
feature branch and master at some point. Not ideal in my mind, but maybe
we need to be more pragmatic with this one.
Do we have other tasks (like these PRs) to complete before we are ready to
consider the merge?
I am just looking to help however I can in killing this feature branch;
meaning I want your code in master, as soon as possible. :)
On Wed, Sep 20, 2017 at 10:27 AM Otto Fowler <ot...@gmail.com>
wrote:
> So, were the functionality is not FB related, I have been doing PR’s
> against master ( such as hdfs service ability to set permissions ).
> I don’t think we have talked about the end game PR from feature to master,
> I am don’t know how we would do multiple PRs to bring it down
> once ‘accepted’. I think that approach needs to to be discussed and
> agreed on.
>
>
> On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org) wrote:
>
> So it sounds like these PRs do move us closer to bringing the two branches
> together. But I think I am missing your high-level approach though.
>
> How are we going to get all of the functionality from the feature branch
> into master? Are you going to bite-off small chunks of the feature branch
> and introduce PRs against master?
>
>
>
> On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> Right now, these two PR’s are stacked, the UI depends on the BundleSystem
>> changes.
>> I would rather bring them down to feature before I do master integration,
>> than integrate master
>> into feature and then bring it out to the stacked PR’s. Simple as that.
>>
>>
>>
>> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org) wrote:
>>
>> Hi Otto -
>>
>> What is the plan for bringing the feature branch and master together? Do
>> these PRs move us closer to bringing the two branches together?
>>
>> Thanks
>>
>>
>>
>> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>> > Can I get a bump on https://github.com/apache/metron/pull/747 and
>> > https://github.com/apache/metron/pull/761?
>> >
>> > The next time I take master is going to be a bit of integration work,
>> and I
>> > would like to unstack and get my stuff integrated first.
>> >
>>
>>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
So, were the functionality is not FB related, I have been doing PR’s
against master ( such as hdfs service ability to set permissions ).
I don’t think we have talked about the end game PR from feature to master,
I am don’t know how we would do multiple PRs to bring it down
once ‘accepted’. I think that approach needs to to be discussed and agreed
on.
On September 20, 2017 at 10:23:44, Nick Allen (nick@nickallen.org) wrote:
So it sounds like these PRs do move us closer to bringing the two branches
together. But I think I am missing your high-level approach though.
How are we going to get all of the functionality from the feature branch
into master? Are you going to bite-off small chunks of the feature branch
and introduce PRs against master?
On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com> wrote:
> Right now, these two PR’s are stacked, the UI depends on the BundleSystem
> changes.
> I would rather bring them down to feature before I do master integration,
> than integrate master
> into feature and then bring it out to the stacked PR’s. Simple as that.
>
>
>
> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org) wrote:
>
> Hi Otto -
>
> What is the plan for bringing the feature branch and master together? Do
> these PRs move us closer to bringing the two branches together?
>
> Thanks
>
>
>
> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
> > Can I get a bump on https://github.com/apache/metron/pull/747 and
> > https://github.com/apache/metron/pull/761?
> >
> > The next time I take master is going to be a bit of integration work,
> and I
> > would like to unstack and get my stuff integrated first.
> >
>
>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
So it sounds like these PRs do move us closer to bringing the two branches
together. But I think I am missing your high-level approach though.
How are we going to get all of the functionality from the feature branch
into master? Are you going to bite-off small chunks of the feature branch
and introduce PRs against master?
On Wed, Sep 20, 2017 at 9:23 AM Otto Fowler <ot...@gmail.com> wrote:
> Right now, these two PR’s are stacked, the UI depends on the BundleSystem
> changes.
> I would rather bring them down to feature before I do master integration,
> than integrate master
> into feature and then bring it out to the stacked PR’s. Simple as that.
>
>
>
> On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org) wrote:
>
> Hi Otto -
>
> What is the plan for bringing the feature branch and master together? Do
> these PRs move us closer to bringing the two branches together?
>
> Thanks
>
>
>
> On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
> > Can I get a bump on https://github.com/apache/metron/pull/747 and
> > https://github.com/apache/metron/pull/761?
> >
> > The next time I take master is going to be a bit of integration work,
> and I
> > would like to unstack and get my stuff integrated first.
> >
>
>
Re: feature branch bumps
Posted by Otto Fowler <ot...@gmail.com>.
Right now, these two PR’s are stacked, the UI depends on the BundleSystem
changes.
I would rather bring them down to feature before I do master integration,
than integrate master
into feature and then bring it out to the stacked PR’s. Simple as that.
On September 20, 2017 at 08:35:09, Nick Allen (nick@nickallen.org) wrote:
Hi Otto -
What is the plan for bringing the feature branch and master together? Do
these PRs move us closer to bringing the two branches together?
Thanks
On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com>
wrote:
> Can I get a bump on https://github.com/apache/metron/pull/747 and
> https://github.com/apache/metron/pull/761?
>
> The next time I take master is going to be a bit of integration work, and
I
> would like to unstack and get my stuff integrated first.
>
Re: feature branch bumps
Posted by Nick Allen <ni...@nickallen.org>.
Hi Otto -
What is the plan for bringing the feature branch and master together? Do
these PRs move us closer to bringing the two branches together?
Thanks
On Wed, Sep 20, 2017 at 8:19 AM Otto Fowler <ot...@gmail.com> wrote:
> Can I get a bump on https://github.com/apache/metron/pull/747 and
> https://github.com/apache/metron/pull/761?
>
> The next time I take master is going to be a bit of integration work, and I
> would like to unstack and get my stuff integrated first.
>