metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shane Ardell <shane.m.ard...@gmail.com>
Subject Re: [DISCUSS] Parser Aggregation in Management UI
Date Wed, 08 May 2019 09:51:47 GMT
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