> 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 <[email protected]> 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 ([email protected]) 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 <[email protected]> > 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 ([email protected]) 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 <[email protected]> >> 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 ([email protected]) >>> 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 <[email protected]> >>> 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 ([email protected]) >>>> 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 <[email protected]> >>>> 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 ([email protected]) >>>>> 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 <[email protected]> >>>>> 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 ([email protected]) >>>>>> 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 <[email protected]> >>>>>> 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. >>>>>> > >>>>>> >>>>>>
