unsubscribe -----Original Message----- From: Tibor Meller [mailto:tibor.mel...@gmail.com] Sent: Monday, July 1, 2019 11:07 AM To: dev@metron.apache.org Subject: Re: [DISCUSS] Parser Aggregation in Management UI
HI, Can we continue the review process with the next PR in the line? https://github.com/apache/metron/pull/1414 First three PR get merged to the feature branch by Ryan. Thanks for that! On Tue, Jun 11, 2019 at 10:20 PM Michael Miklavcic < michael.miklav...@gmail.com> wrote: > Agreed Ryan, I know this is a bit of a cart before the horse scenario. > > This isn't the most desirable of circumstances, and we're doing our > best to accommodate the contribution because it's a good feature to > have, and the work so far looks pretty solid. We have a bit more > flexibility in a feature branch than we would if this was (as it was > originally) submitted against master - I think the most important > thing here is we do architecture/code/test review on an individual PR > basis. They've split up this work pretty fine-grained, so I don't > think it should be too hard for us to work through. When we get > through PRs 1-14 from a code review perspective, we'll have to do a > run of manual acceptance tests on the final product before accepting > the feature branch into master. Anything that appears to be broken or > missing will simply need another PR to address it, and we can get it > in. I actually don't think this should be too bad. Again, thank you to > Shane, Tibor, and Tamas for breaking this down. Just be careful in the > future when you're collaborating on something of this size that you either A) > split off the work in fully functioning PRs, e.g. > refactoring work or library changes can very easily be submitted > against master without breaking anything or B) start with a feature > branch and discuss thread that lays out your plan for delivering the > feature in that FB. > > Please review Apache's guide on consensus building, as well - > http://community.apache.org/committers/consensusBuilding.html > > On Tue, Jun 11, 2019 at 12:01 PM Ryan Merriman <merrim...@gmail.com> > wrote: > > > "We planning to add the changes to the latest PR as additional > > commits to avoid impacting the PR sequence. We will refer to the > > source PR in the commit message of the fix. Also adding a link to > > the comment section of > the > > source PR of the change request to the fixing commit to make them > > connected." > > > > I don't think this is going to work. If changes are requested and > applied > > in a different PR, how would you test the original PR? What would > > you do if there were merge conflicts introduced between the current > > and final > PR? > > What's the point of even having any PRs before the final one if they > won't > > get committed or changed? I don't see any way to avoid having to > propagate > > changes through all subsequent PRs. > > > > On Wed, May 29, 2019 at 7:03 AM Tibor Meller > > <tibor.mel...@gmail.com> > > wrote: > > > > > Hi all, > > > > > > *We still need some volunteer reviewers for this feature.* All > individual > > > PR is under 1000 line of changes except one and it is due to an > > > autogenerated package.lock.json file. > > > Just a heads up: parser aggregation by default turned on for bro, > > > snort > > and > > > yarn parser on full dev. Without this changeset, full dev is broken. > > > > > > > > > https://lists.apache.org/thread.html/beeb4cfddfca7958a22ab926f72f52f46 > a33c42edce714112df9a2da@%3Cdev.metron.apache.org%3E > > > > > > > > > > > > On Fri, May 24, 2019 at 3:20 PM Tibor Meller > > > <tibor.mel...@gmail.com> > > > wrote: > > > > > > > Please find below the list of the PRs we opened for Parser > Aggregation. > > > > With Shane, Tamas we tried to provide as much information as > > > > possible > > to > > > > make the reviewing process easier. > > > > Please keep that in mind these PRs are not against muster but a > Parser > > > > Aggregation feature branch. > > > > If you like to read more about the process we followed with > > > > these PRs please read the previous three message in this thread. > > > > > > > > PR#1 METRON-2114: [UI] Moving components to sensor parser module > > > > <https://github.com/apache/metron/pull/1410> > > > > PR#2 METRON-2116: [UI] Removing redundant AppConfigService > > > > <https://github.com/apache/metron/pull/1411> > > > > PR#3 METRON-2117: [UI] Aligning models to grouping feature > > > > <https://github.com/apache/metron/pull/1412> > > > > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP > > > > <https://github.com/apache/metron/pull/1414> > > > > PR#5 METRON-2122: [UI] Fixing early app config access issue > > > > <https://github.com/apache/metron/pull/1415> > > > > PR#6 METRON-2124: [UI] Move status information and start/stop to > > > > the Aggregate level <https://github.com/apache/metron/pull/1418> > > > > PR#7 METRON-2125: [UI] Making changes visible in the parser list > > > > by marking changed items > > > > <https://github.com/apache/metron/pull/1422> > > > > PR#8 METRON-2131: Add NgRx and related dependencies > > > > <https://github.com/apache/metron/pull/1423> > > > > PR#9 METRON-2133: Add NgRx effects to communicate with the > > > > server <https://github.com/apache/metron/pull/1424> > > > > PR#10 METRON-2134: Add NgRx reducers to perform parser and group > > changes > > > > in the store <https://github.com/apache/metron/pull/1425> > > > > PR#11 METRON-2135: Add NgRx actions to trigger state changes > > > > <https://github.com/apache/metron/pull/1426> > > > > PR#12 METRON-2136: Add parser aggregation sidebar > > > > <https://github.com/apache/metron/pull/1427> > > > > PR#13 METRON-2137: Implement drag and drop mechanism and wire > > > > NgRx <https://github.com/apache/metron/pull/1428> > > > > PR#14 METRON-2138: Code clean up > > > > <https://github.com/apache/metron/pull/1429> > > > > PR#15 METRON-2139: Refactoring sensor-parser-config.component > > > > and > wire > > > > NgRx <https://github.com/apache/metron/pull/1430> > > > > > > > > Thanks, > > > > Tibor > > > > > > > > > > > > On Thu, May 23, 2019 at 11:45 AM Tibor Meller < > tibor.mel...@gmail.com> > > > > wrote: > > > > > > > >> Yes, am expecting that some change request will rase due to the > > review. > > > >> We planning to add the changes to the latest PR as additional > commits > > to > > > >> avoid impacting the PR sequence. We will refer to the source PR > > > >> in > the > > > >> commit message of the fix. Also adding a link to the comment > > > >> section > > of > > > the > > > >> source PR of the change request to the fixing commit to make > > > >> them > > > connected. > > > >> > > > >> On Wed, May 22, 2019 at 5:49 PM Michael Miklavcic < > > > >> michael.miklav...@gmail.com> wrote: > > > >> > > > >>> Tibor, that sounds reasonable to me. If PR #1 ends up > > > >>> requiring > code > > > >>> changes, will you guys just percolate those up through the > remaining > > k > > > >>> PRs > > > >>> in order, or just the final PR? I'm wondering how this works > > > >>> in > > > reference > > > >>> to your last point in #5 about rebasing. > > > >>> > > > >>> On Wed, May 22, 2019 at 8:47 AM Tibor Meller < > tibor.mel...@gmail.com > > > > > > >>> wrote: > > > >>> > > > >>> > I would like to describe quickly *our approach to breaking > > > >>> > down > > > Parser > > > >>> > Aggregation PR for smaller chunks* > > > >>> > > > > >>> > *1. we squashed the commits in the original development > > > >>> > branch* > > > >>> > - when we started to open smaller PRs from the commits from > > > >>> > the > > > >>> original > > > >>> > branch, we found ourself opening PRs out of historical > > > >>> > states of > > the > > > >>> code > > > >>> > instead of the final one > > > >>> > - none of those states of development are worth (or make > > > >>> > sense) > to > > be > > > >>> > reviewed (initial phases of development are included in the > > original > > > >>> commit > > > >>> > history, multiple iterations of refactoring, etc.) > > > >>> > - while the actual development history was irrelevant, the > > > attribution > > > >>> > aspect of it was still important *2. we divided** the > > > >>> > changes by the original authors* > > > >>> > - the original contributors were sardell, ruffle1986 and > > > >>> > tiborm > > > >>> > - we isolated the changes that belong to each individual > > contributor > > > >>> > *3. each of us identified smaller but belonging changesets * > > > >>> > - with this, we ended up opening 5 PRs from tiborm, 3 from > sardell > > > and > > > >>> 6 > > > >>> > from ruffle1986 > > > >>> > - each of these are smaller than 500 lines of changes, which > makes > > > the > > > >>> task > > > >>> > of reviewing easier > > > >>> > - they have their own context and purpose described by the > > > >>> > PR and > > the > > > >>> > related Jira ticket > > > >>> > *4. Each PR introduces a single new commit which is meant to > > > >>> > be > > > >>> reviewed* > > > >>> > - with this we were able to open PRs on top of each others > > > >>> > work, > > but > > > >>> the > > > >>> > reviewer is still able to identify what changes were > > > >>> > introduced > and > > > >>> > described by the pr simply by focusing on the last commit > > > >>> > - the commit introduced by the PR has the same commit > > > >>> > message as > > the > > > >>> title > > > >>> > of the PR to make it easier to find *5. Only the last PR is > > > >>> > meant to be merged into the feature > branch* > > > >>> > - the last PR also introduces a single new commit to being > reviewed > > > >>> > - this contains all the commits from the previous PRs that > > > >>> > belong > > to > > > >>> parser > > > >>> > aggregation > > > >>> > - it builds fine in Travis > > > >>> > - it's fully functional and ready to being tested against > > > >>> > full > dev > > > >>> > - If we only merge the last PR, we don't have to rebase and > > recreate > > > >>> all of > > > >>> > our PRs due to merge conflicts that will result from to > conflicting > > > >>> > histories (which is common in feature branch work) > > > >>> > > > > >>> > Once all the Pull Requests are open, I will submit a list of > > > >>> > all > of > > > >>> them to > > > >>> > this discussion thread. > > > >>> > > > > >>> > On Wed, May 8, 2019 at 3:58 PM Otto Fowler < > > ottobackwa...@gmail.com> > > > >>> > wrote: > > > >>> > > > > >>> > > You need to be a committer, that is all I think. > > > >>> > > I would not use the github UI for it though, I do it > > > >>> > > through > the > > > cli > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > On May 8, 2019 at 09:45:24, Michael Miklavcic ( > > > >>> > michael.miklav...@gmail.com > > > >>> > > ) > > > >>> > > wrote: > > > >>> > > > > > >>> > > Not that I'm aware of. Nick and Otto, you've created them > before, > > > >>> did you > > > >>> > > need any special perms? > > > >>> > > > > > >>> > > On Wed, May 8, 2019 at 3:57 AM Shane Ardell < > > > >>> shane.m.ard...@gmail.com> > > > >>> > > wrote: > > > >>> > > > > > >>> > > > This morning, we started to break down our work as > > > >>> > > > Michael > > > >>> suggested in > > > >>> > > > this thread. However, it looks like I don't have > > > >>> > > > permission > to > > > >>> create a > > > >>> > > new > > > >>> > > > branch in the GitHub UI or push a new branch to the > > apache/metron > > > >>> repo. > > > >>> > > Is > > > >>> > > > this action restricted to PMC members only? > > > >>> > > > > > > >>> > > > Shane > > > >>> > > > > > > >>> > > > On Wed, May 8, 2019 at 9:06 AM Tamás Fodor < > > > ftamas.m...@gmail.com> > > > >>> > > wrote: > > > >>> > > > > > > >>> > > > > Here's the process we've gone through in order to > > > >>> > > > > implement > > the > > > >>> > > feature. > > > >>> > > > > > > > >>> > > > > At the beginning we had some bootstrap work like > > > >>> > > > > creating a > > > mock > > > >>> API > > > >>> > > > > (written in NodeJS) because we were a few steps ahead > > > >>> > > > > the > > > backend > > > >>> > part. > > > >>> > > > But > > > >>> > > > > this is in a totally different repository so it > > > >>> > > > > doesn't > > really > > > >>> count. > > > >>> > > We > > > >>> > > > > also had to wire NgRX, our chosen 3rd party that > > > >>> > > > > supports > the > > > >>> flux > > > >>> > flow > > > >>> > > > to > > > >>> > > > > get a better state management. When we were ready to > > > >>> > > > > kick > off > > > >>> > > > implementing > > > >>> > > > > the business logic in, we splited up the work by > subfeatures > > > like > > > >>> > drag > > > >>> > > > and > > > >>> > > > > dropping table rows. At this point, we created a POC > without > > > NgRX > > > >>> > just > > > >>> > > to > > > >>> > > > > let you have the feeling of how it works in real life. > Later > > > on, > > > >>> > after > > > >>> > > > > introducing NgRX, we had to refactor it a little bit > > obviously > > > to > > > >>> > make > > > >>> > > it > > > >>> > > > > compatible with NgRX. There were other subfeatures > > > >>> > > > > like > > > creating > > > >>> and > > > >>> > > > > editing groups in a floating pane on the right side of > > > >>> > > > > the > > > >>> window. > > > >>> > > > > When the real backend API was ready we made the > > > >>> > > > > necessary > > > >>> changes and > > > >>> > > > > tested whether it worked how it was expected. There > > > >>> > > > > were a > > few > > > >>> > > difference > > > >>> > > > > between how we originally planned the API and the > > > >>> > > > > current > > > >>> > > implementation > > > >>> > > > so > > > >>> > > > > we had to adapt it accordingly. While we were > > > >>> > > > > implementing > > the > > > >>> > > features, > > > >>> > > > we > > > >>> > > > > wrote the unit tests simultaneously. The latest task > > > >>> > > > > on the > > > >>> feature > > > >>> > was > > > >>> > > > > restricting the user from aggregating parsers together. > > > >>> > > > > > > > >>> > > > > As a first iteration, we've decided to put the > > > >>> > > > > restriction > in > > > >>> because > > > >>> > > it > > > >>> > > > > requires a bigger effort on the backend to deal with that. > In > > > my > > > >>> > > opinion, > > > >>> > > > > we should get rid of the restriction because it's not > > intuitive > > > >>> and > > > >>> > > very > > > >>> > > > > inconvenient. In my opinion, we should let the users > > > >>> > > > > to > > > >>> aggregate the > > > >>> > > > > running parsers together and do the job to handle this > > > >>> > > > > edge > > > case > > > >>> on > > > >>> > the > > > >>> > > > > backend accordingly. > > > >>> > > > > > > > >>> > > > > What do you think, guys? > > > >>> > > > > > > > >>> > > > > Hope this helps. > > > >>> > > > > > > > >>> > > > > Tamas > > > >>> > > > > > > > >>> > > > > On Tue, May 7, 2019 at 4:34 PM Michael Miklavcic < > > > >>> > > > > michael.miklav...@gmail.com> wrote: > > > >>> > > > > > > > >>> > > > > > This was my expectation as well. > > > >>> > > > > > > > > >>> > > > > > Shane, Tibor, Tamas - how did you go about breaking > > > >>> > > > > > this > > down > > > >>> into > > > >>> > > > chunks > > > >>> > > > > > and/or microchunks when you collaborated offline? As > > > >>> > > > > > Nick > > > >>> > mentioned, > > > >>> > > > you > > > >>> > > > > > obviously split up work and shared it amongst yourselves. > > > Some > > > >>> > > > > explanation > > > >>> > > > > > around this process would be helpful for reviewers > > > >>> > > > > > as > well. > > > We > > > >>> > might > > > >>> > > be > > > >>> > > > > > able to provide better guidance and examples to > > > >>> > > > > > future > > > >>> contributors > > > >>> > > as > > > >>> > > > > > well. > > > >>> > > > > > > > > >>> > > > > > I talked a little bit with Shane about this offline > > > >>> > > > > > last > > > week. > > > >>> It > > > >>> > > looks > > > >>> > > > > > like you guys effectively ran a local feature branch. > > > >>> Replicating > > > >>> > > that > > > >>> > > > > > process in a feature branch in Apache is probably > > > >>> > > > > > what > you > > > guys > > > >>> > > should > > > >>> > > > > > be doing for a change this size. We don't have hard > limits > > on > > > >>> line > > > >>> > > > change > > > >>> > > > > > size, but in the past it's been somewhere around > > > >>> > > > > > 2k-3k > > lines > > > >>> and > > > >>> > > above > > > >>> > > > > > being the tipping point for discussing a feature branch. > > > >>> Strictly > > > >>> > > > > speaking, > > > >>> > > > > > line quantity alone is not the only metric, but it's > > relevant > > > >>> here. > > > >>> > > If > > > >>> > > > > you > > > >>> > > > > > want to make smaller incremental changes locally, > > > >>> > > > > > there's > > > >>> nothing > > > >>> > to > > > >>> > > > keep > > > >>> > > > > > you from doing that - I would only advise that you > consider > > > >>> > squashing > > > >>> > > > > those > > > >>> > > > > > commits (just ask if you're unclear about how to > > > >>> > > > > > handle > > that) > > > >>> into > > > >>> > a > > > >>> > > > > single > > > >>> > > > > > larger commit/chunk when you're ready to publish > > > >>> > > > > > them as > a > > > >>> chunk to > > > >>> > > the > > > >>> > > > > > public feature branch. So it would look something > > > >>> > > > > > like > > this: > > > >>> > > > > > > > > >>> > > > > > Commits by person locally > > > >>> > > > > > Shane: 1,2,3 -> squash as A > > > >>> > > > > > Tibor: 4,5,6 -> squash as B > > > >>> > > > > > Tamas: 7,8,9 -> squash as C > > > >>> > > > > > > > > >>> > > > > > Commits by person in Apache > > > >>> > > > > > Shane: A > > > >>> > > > > > Tibor: B > > > >>> > > > > > Tamas: C > > > >>> > > > > > > > > >>> > > > > > We need to maintain a record of attribution. Your > > > >>> > > > > > real > > > >>> workflow may > > > >>> > > not > > > >>> > > > > be > > > >>> > > > > > that cleanly delineated, but you can choose how you > > > >>> > > > > > want > to > > > >>> squash > > > >>> > in > > > >>> > > > > those > > > >>> > > > > > cases. Even in public collaboration, there are > > > >>> > > > > > plenty of > > > cases > > > >>> > where > > > >>> > > > > folks > > > >>> > > > > > submit PRs against PRs, abstain from accepting > attribution, > > > >>> and it > > > >>> > > all > > > >>> > > > > gets > > > >>> > > > > > squashed down into one person's final PR commit. > > > >>> > > > > > There > are > > > many > > > >>> > > > options. > > > >>> > > > > > > > > >>> > > > > > Hope this helps. > > > >>> > > > > > > > > >>> > > > > > Best, > > > >>> > > > > > Mike > > > >>> > > > > > > > > >>> > > > > > On Mon, May 6, 2019 at 8:19 AM Nick Allen < > > > n...@nickallen.org> > > > >>> > > wrote: > > > >>> > > > > > > > > >>> > > > > > > Have you considered creating a feature branch for > > > >>> > > > > > > the > > > effort? > > > >>> > This > > > >>> > > > > would > > > >>> > > > > > > allow you to break the effort into chunks, where > > > >>> > > > > > > the > > result > > > >>> of > > > >>> > each > > > >>> > > > PR > > > >>> > > > > > may > > > >>> > > > > > > not be a fully working "master-ready" result. > > > >>> > > > > > > > > > >>> > > > > > > I am sure you guys tackled the work in chunks when > > > >>> developing it, > > > >>> > > so > > > >>> > > > > > > consider just replaying those chunks onto the > > > >>> > > > > > > feature > > > branch > > > >>> as > > > >>> > > > > separate > > > >>> > > > > > > PRs. > > > >>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > On Mon, May 6, 2019 at 5:24 AM Tibor Meller < > > > >>> > > tibor.mel...@gmail.com> > > > >>> > > > > > >>> > > > > > > wrote: > > > >>> > > > > > > > > > >>> > > > > > > > I wondered on the weekend how we could split > > > >>> > > > > > > > that PR > to > > > >>> smaller > > > >>> > > > > chunks. > > > >>> > > > > > > > That PR is a result of almost 2 months of > > > >>> > > > > > > > development > > > and I > > > >>> > don't > > > >>> > > > see > > > >>> > > > > > how > > > >>> > > > > > > > to split that to multiple WORKING parts. It is > > > >>> > > > > > > > as it > > is a > > > >>> whole > > > >>> > > > > working > > > >>> > > > > > > > feature. If we split it by packages or files we > > > >>> > > > > > > > could > > > >>> provide > > > >>> > > > smaller > > > >>> > > > > > > > non-functional PR's, but can end up having a > > > >>> > > > > > > > broken > > > >>> Management > > > >>> > UI > > > >>> > > > > after > > > >>> > > > > > > > having the 1st PR part merged into master. I > > > >>> > > > > > > > don't > > think > > > >>> that > > > >>> > > would > > > >>> > > > > be > > > >>> > > > > > > > acceptable by the community (or even by me) so I > would > > > >>> like to > > > >>> > > > > suggest > > > >>> > > > > > > two > > > >>> > > > > > > > other option to help review PR#1360. > > > >>> > > > > > > > > > > >>> > > > > > > > #1 We could extend that PR with our own author > comments > > > in > > > >>> > > Github. > > > >>> > > > > That > > > >>> > > > > > > > would help following which code part belongs to > > > >>> > > > > > > > where > > and > > > >>> why > > > >>> > it > > > >>> > > > was > > > >>> > > > > > > > necessary. > > > >>> > > > > > > > #2 We can schedule an interactive code > > > >>> > > > > > > > walkthrough > call > > > >>> with > > > >>> > the > > > >>> > > > ones > > > >>> > > > > > who > > > >>> > > > > > > > interested in reviewing or the particular changeset. > > > >>> > > > > > > > > > > >>> > > > > > > > Please share your thoughts on this! Which > > > >>> > > > > > > > version > would > > > >>> support > > > >>> > > you > > > >>> > > > > the > > > >>> > > > > > > > best? Or if you have any other idea let us know. > > > >>> > > > > > > > > > > >>> > > > > > > > PS: I think the size of our PR's depends on how > > > >>> > > > > > > > small > > > >>> > > independently > > > >>> > > > > > > > deliverable changesets we can identify before we > > starting > > > >>> to > > > >>> > > > > implement > > > >>> > > > > > a > > > >>> > > > > > > > relatively big new feature. Unfortunately, we > > > >>> > > > > > > > missed > to > > > do > > > >>> that > > > >>> > > > with > > > >>> > > > > > this > > > >>> > > > > > > > feature. > > > >>> > > > > > > > > > > >>> > > > > > > > On Fri, May 3, 2019 at 1:49 PM Shane Ardell < > > > >>> > > > > shane.m.ard...@gmail.com> > > > >>> > > > > > > > wrote: > > > >>> > > > > > > > > > > >>> > > > > > > > > NgRx was only used for the aggregation feature > > > >>> > > > > > > > > and > > > >>> doesn't go > > > >>> > > > > beyond > > > >>> > > > > > > > that. > > > >>> > > > > > > > > I think the way I worded that sentence may > > > >>> > > > > > > > > have > > caused > > > >>> > > > confusion. I > > > >>> > > > > > > just > > > >>> > > > > > > > > meant we use it to manage more pieces of state > within > > > the > > > >>> > > > > aggregation > > > >>> > > > > > > > > feature than just previous and current state > > > >>> > > > > > > > > of > > grouped > > > >>> > > parsers. > > > >>> > > > > > > > > > > > >>> > > > > > > > > On Fri, May 3, 2019 at 1:32 AM Michael > > > >>> > > > > > > > > Miklavcic < michael.miklav...@gmail.com> wrote: > > > >>> > > > > > > > > > > > >>> > > > > > > > > > Shane, thanks for putting this together. The > > updates > > > >>> on the > > > >>> > > > Jira > > > >>> > > > > > are > > > >>> > > > > > > > > useful > > > >>> > > > > > > > > > as well. > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > (we used it for more than just that in > > > >>> > > > > > > > > > > this > > > feature, > > > >>> but > > > >>> > > that > > > >>> > > > > was > > > >>> > > > > > > the > > > >>> > > > > > > > > > initial reasoning) What are you using NgRx > > > >>> > > > > > > > > > for in the submitted work > > > that > > > >>> goes > > > >>> > > > > beyond > > > >>> > > > > > > the > > > >>> > > > > > > > > > aggregation feature? > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > On Thu, May 2, 2019 at 12:22 PM Shane Ardell > > > >>> > > > > > > > > > < > > > >>> > > > > > > shane.m.ard...@gmail.com > > > >>> > > > > > > > > > > > >>> > > > > > > > > > wrote: > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > Hello everyone, > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > In response to discussions in the 0.7.1 > > > >>> > > > > > > > > > > release > > > >>> thread, I > > > >>> > > > > wanted > > > >>> > > > > > to > > > >>> > > > > > > > > > start a > > > >>> > > > > > > > > > > thread regarding the parser aggregation > > > >>> > > > > > > > > > > work > for > > > the > > > >>> > > > Management > > > >>> > > > > > UI. > > > >>> > > > > > > > For > > > >>> > > > > > > > > > > anyone who has not already read and tested > > > >>> > > > > > > > > > > the > PR > > > >>> > locally, > > > >>> > > > I've > > > >>> > > > > > > > added a > > > >>> > > > > > > > > > > detailed description of what we did and > > > >>> > > > > > > > > > > why to > > the > > > >>> JIRA > > > >>> > > > ticket > > > >>> > > > > > > here: > > > >>> > > > > > > > > > > > > https://issues.apache.org/jira/browse/METRON-1856 > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > I'm wondering what the community thinks > > > >>> > > > > > > > > > > about > > what > > > >>> we've > > > >>> > > > built > > > >>> > > > > > thus > > > >>> > > > > > > > > far. > > > >>> > > > > > > > > > Do > > > >>> > > > > > > > > > > you see anything missing that must be part > > > >>> > > > > > > > > > > of > > this > > > >>> new > > > >>> > > > feature > > > >>> > > > > in > > > >>> > > > > > > the > > > >>> > > > > > > > > UI? > > > >>> > > > > > > > > > > Are there any strong objections to how we > > > >>> implemented it? > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > I’m also looking to see if anyone has any > > thoughts > > > >>> on how > > > >>> > > we > > > >>> > > > > can > > > >>> > > > > > > > > possibly > > > >>> > > > > > > > > > > simplify this PR. Right now it's pretty > > > >>> > > > > > > > > > > big, > and > > > >>> there > > > >>> > are > > > >>> > > a > > > >>> > > > > lot > > > >>> > > > > > of > > > >>> > > > > > > > > > commits > > > >>> > > > > > > > > > > to parse through, but I'm not sure how we > > > >>> > > > > > > > > > > could > > > break > > > >>> > this > > > >>> > > > work > > > >>> > > > > > out > > > >>> > > > > > > > > into > > > >>> > > > > > > > > > > separate, smaller PRs opened against > > > >>> > > > > > > > > > > master. We > > > >>> could try > > > >>> > > to > > > >>> > > > > > > > > cherry-pick > > > >>> > > > > > > > > > > the commits into smaller PRs and then > > > >>> > > > > > > > > > > merge > them > > > >>> into a > > > >>> > > > feature > > > >>> > > > > > > > branch, > > > >>> > > > > > > > > > but > > > >>> > > > > > > > > > > I'm not sure if that's worth the effort > > > >>> > > > > > > > > > > since > > that > > > >>> will > > > >>> > > only > > > >>> > > > > > reduce > > > >>> > > > > > > > the > > > >>> > > > > > > > > > > number commits to review, not the lines > changed. > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > As an aside, I also want to give a little > > > background > > > >>> into > > > >>> > > the > > > >>> > > > > > > > > > introduction > > > >>> > > > > > > > > > > of NgRx in this PR. To give a little > > > >>> > > > > > > > > > > background > > on > > > >>> why we > > > >>> > > > chose > > > >>> > > > > > to > > > >>> > > > > > > do > > > >>> > > > > > > > > > this, > > > >>> > > > > > > > > > > you can refer to the discussion thread here: > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > > >>> > > > > >>> > > > > > > https://lists.apache.org/thread.html/06a59ea42e8d9a9dea5f90aab4011e444 > 34555f8b7f3cf21297c7c87@%3Cdev.metron.apache.org%3E > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > We previously discussed introducing a > > > >>> > > > > > > > > > > better > way > > to > > > >>> > manage > > > >>> > > > > > > > application > > > >>> > > > > > > > > > > state in both UIs in that thread. It was > decided > > > that > > > >>> > NgRx > > > >>> > > > was > > > >>> > > > > a > > > >>> > > > > > > > great > > > >>> > > > > > > > > > tool > > > >>> > > > > > > > > > > for many reasons, one of them being that > > > >>> > > > > > > > > > > we can > > > >>> piecemeal > > > >>> > > it > > > >>> > > > > into > > > >>> > > > > > > the > > > >>> > > > > > > > > > > application rather than doing a huge > > > >>> > > > > > > > > > > rewrite of > > all > > > >>> the > > > >>> > > > > > application > > > >>> > > > > > > > > state > > > >>> > > > > > > > > > > at once. The contributors in this PR > > > >>> > > > > > > > > > > (myself > > > >>> included) > > > >>> > > > decided > > > >>> > > > > > this > > > >>> > > > > > > > > would > > > >>> > > > > > > > > > > be a perfect opportunity to introduce NgRx > > > >>> > > > > > > > > > > into > > the > > > >>> > > > Management > > > >>> > > > > UI > > > >>> > > > > > > > since > > > >>> > > > > > > > > > we > > > >>> > > > > > > > > > > need to manage the previous and current > > > >>> > > > > > > > > > > state > > with > > > >>> the > > > >>> > > > grouping > > > >>> > > > > > > > feature > > > >>> > > > > > > > > > so > > > >>> > > > > > > > > > > that users can undo the changes they've > > > >>> > > > > > > > > > > made > (we > > > >>> used it > > > >>> > > for > > > >>> > > > > more > > > >>> > > > > > > > than > > > >>> > > > > > > > > > just > > > >>> > > > > > > > > > > that in this feature, but that was the > > > >>> > > > > > > > > > > initial > > > >>> > reasoning). > > > >>> > > In > > > >>> > > > > > > > addition, > > > >>> > > > > > > > > > we > > > >>> > > > > > > > > > > greatly benefited from this when it came > > > >>> > > > > > > > > > > time > to > > > >>> debug > > > >>> > our > > > >>> > > > work > > > >>> > > > > > in > > > >>> > > > > > > > the > > > >>> > > > > > > > > UI > > > >>> > > > > > > > > > > (the discussion in the above thread link > > > >>> > > > > > > > > > > goes a > > > >>> little > > > >>> > more > > > >>> > > > > into > > > >>> > > > > > > the > > > >>> > > > > > > > > > > advantages of debugging with NgRx and > DevTools). > > > >>> Removing > > > >>> > > > NgRx > > > >>> > > > > > from > > > >>> > > > > > > > > this > > > >>> > > > > > > > > > > work would reduce the numbers of lines > > > >>> > > > > > > > > > > changed > > > >>> slightly, > > > >>> > > but > > > >>> > > > it > > > >>> > > > > > > would > > > >>> > > > > > > > > > still > > > >>> > > > > > > > > > > be a big PR and a lot of that code would > > > >>> > > > > > > > > > > just > > move > > > >>> to the > > > >>> > > > > > component > > > >>> > > > > > > > or > > > >>> > > > > > > > > > > service level in the Angular application. > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > Shane > > > >>> > > > > > > > > > > > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >> > > > > > >
