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 Thu, 23 May 2019 09:45:38 GMT
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