metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Miklavcic <michael.miklav...@gmail.com>
Subject Re: [DISCUSS] Parser Aggregation in Management UI
Date Wed, 22 May 2019 15:48:49 GMT
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