metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tibor Meller <tibor.mel...@gmail.com>
Subject Re: [DISCUSS] Parser Aggregation in Management UI
Date Mon, 06 May 2019 09:53:21 GMT
@Otto: I responded to your questions in a few Jira comment.

On Mon, May 6, 2019 at 11:21 AM Tibor Meller <tibor.meller@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.ardell@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.miklavcic@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.ardell@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
>> > >
>> >
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message