> 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 <ottobackwa...@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 (n...@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 <ottobackwa...@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 (n...@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 <ottobackwa...@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 (n...@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 <ottobackwa...@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 (n...@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 <ottobackwa...@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 (n...@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 <ottobackwa...@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 (n...@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 <ottobackwa...@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.
>>>>>> >
>>>>>>
>>>>>>

Reply via email to