@Otto: I responded to your questions in a few Jira comment. On Mon, May 6, 2019 at 11:21 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/06a59ea42e8d9a9dea5f90aab4011e44434555f8b7f3cf21297c7c87@%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 >> > > >> > >> >
