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 Wed, 29 May 2019 12:02:12 GMT
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/beeb4cfddfca7958a22ab926f72f52f46a33c42edce714112df9a2da@%3Cdev.metron.apache.org%3E



On Fri, May 24, 2019 at 3:20 PM Tibor Meller <tibor.meller@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.meller@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.miklavcic@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.meller@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 <ottobackwards@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.miklavcic@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.ardell@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.mail@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.miklavcic@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 <nick@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.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