RE: [DISCUSS] Parser Aggregation in Management UI

[Ian Abreu]

unsubscribe

-Original Message-
From: Tibor Meller [mailto:tibor.mel...@gmail.com] 
Sent: Monday, July 1, 2019 11:07 AM
To: dev@metron.apache.org
Subject: Re: [DISCUSS] Parser Aggregation in Management UI

HI,
Can we continue the review process with the next PR in the line?
https://github.com/apache/metron/pull/1414

First three PR get merged to the feature branch by Ryan. Thanks for that!

On Tue, Jun 11, 2019 at 10:20 PM Michael Miklavcic < 
michael.miklav...@gmail.com> wrote:

> Agreed Ryan, I know this is a bit of a cart before the horse scenario.
>
> This isn't the most desirable of circumstances, and we're doing our 
> best to accommodate the contribution because it's a good feature to 
> have, and the work so far looks pretty solid. We have a bit more 
> flexibility in a feature branch than we would if this was (as it was 
> originally) submitted against master - I think the most important 
> thing here is we do architecture/code/test review on an individual PR 
> basis. They've split up this work pretty fine-grained, so I don't 
> think it should be too hard for us to work through. When we get 
> through PRs 1-14 from a code review perspective, we'll have to do a 
> run of manual acceptance tests on the final product before accepting 
> the feature branch into master. Anything that appears to be broken or 
> missing will simply need another PR to address it, and we can get it 
> in. I actually don't think this should be too bad. Again, thank you to 
> Shane, Tibor, and Tamas for breaking this down. Just be careful in the 
> future when you're collaborating on something of this size that you either A) 
> split off the work in fully functioning PRs, e.g.
> refactoring work or library changes can very easily be submitted 
> against master without breaking anything or B) start with a feature 
> branch and discuss thread that lays out your plan for delivering the 
> feature in that FB.
>
> Please review Apache's guide on consensus building, as well - 
> http://community.apache.org/committers/consensusBuilding.html
>
> On Tue, Jun 11, 2019 at 12:01 PM Ryan Merriman 
> wrote:
>
> > "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."
> >
> > I don't think this is going to work.  If changes are requested and
> applied
> > in a different PR, how would you test the original PR?  What would 
> > you do if there were merge conflicts introduced between the current 
> > and final
> PR?
> > What's the point of even having any PRs before the final one if they
> won't
> > get committed or changed?  I don't see any way to avoid having to
> propagate
> > changes through all subsequent PRs.
> >
> > On Wed, May 29, 2019 at 7:03 AM Tibor Meller 
> > 
> > wrote:
> >
> > > 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/beeb4cfddfca7958a22ab926f72f52f46
> a33c42edce714112df9a2da@%3Cdev.metron.apache.org%3E
> > >
> > >
> > >
> > > On Fri, May 24, 2019 at 3:20 PM Tibor Meller 
> > > 
> > > 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 f

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

HI,
Can we continue the review process with the next PR in the line?
https://github.com/apache/metron/pull/1414

First three PR get merged to the feature branch by Ryan. Thanks for that!

On Tue, Jun 11, 2019 at 10:20 PM Michael Miklavcic <
michael.miklav...@gmail.com> wrote:

> Agreed Ryan, I know this is a bit of a cart before the horse scenario.
>
> This isn't the most desirable of circumstances, and we're doing our best to
> accommodate the contribution because it's a good feature to have, and the
> work so far looks pretty solid. We have a bit more flexibility in a feature
> branch than we would if this was (as it was originally) submitted against
> master - I think the most important thing here is we do
> architecture/code/test review on an individual PR basis. They've split up
> this work pretty fine-grained, so I don't think it should be too hard for
> us to work through. When we get through PRs 1-14 from a code review
> perspective, we'll have to do a run of manual acceptance tests on the final
> product before accepting the feature branch into master. Anything that
> appears to be broken or missing will simply need another PR to address it,
> and we can get it in. I actually don't think this should be too bad. Again,
> thank you to Shane, Tibor, and Tamas for breaking this down. Just be
> careful in the future when you're collaborating on something of this size
> that you either A) split off the work in fully functioning PRs, e.g.
> refactoring work or library changes can very easily be submitted against
> master without breaking anything or B) start with a feature branch and
> discuss thread that lays out your plan for delivering the feature in that
> FB.
>
> Please review Apache's guide on consensus building, as well -
> http://community.apache.org/committers/consensusBuilding.html
>
> On Tue, Jun 11, 2019 at 12:01 PM Ryan Merriman 
> wrote:
>
> > "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."
> >
> > I don't think this is going to work.  If changes are requested and
> applied
> > in a different PR, how would you test the original PR?  What would you do
> > if there were merge conflicts introduced between the current and final
> PR?
> > What's the point of even having any PRs before the final one if they
> won't
> > get committed or changed?  I don't see any way to avoid having to
> propagate
> > changes through all subsequent PRs.
> >
> > On Wed, May 29, 2019 at 7:03 AM Tibor Meller 
> > wrote:
> >
> > > 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 
> > > 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
> > > > 
> > > > PR#2 METRON-2116: [UI] Removing redundant AppConfigService
> > > > 
> > > > PR#3 METRON-2117: [UI] Aligning models to grouping feature
> > > > 
> > > > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP
> > > > 
> > > > PR#5 METRON-2122: [UI] Fixing early app config access issue
> > > > 
> > > > PR#6 METRON-2124: [UI] Move status information and start/stop to the
> > > > Aggregate level 
> > > > PR#7 METRON-2125: [UI] Making changes visible in the parser list by
> > > > marking changed items 
> > > > PR#8 METRON-2131: Add NgRx and related dependencies
> > > > 
> > > > PR#9 METRON-2133: Add NgRx effects to communicate with the server
> > > > 
> > > > PR#10 METRON-2134: Add NgRx 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

Agreed Ryan, I know this is a bit of a cart before the horse scenario.

This isn't the most desirable of circumstances, and we're doing our best to
accommodate the contribution because it's a good feature to have, and the
work so far looks pretty solid. We have a bit more flexibility in a feature
branch than we would if this was (as it was originally) submitted against
master - I think the most important thing here is we do
architecture/code/test review on an individual PR basis. They've split up
this work pretty fine-grained, so I don't think it should be too hard for
us to work through. When we get through PRs 1-14 from a code review
perspective, we'll have to do a run of manual acceptance tests on the final
product before accepting the feature branch into master. Anything that
appears to be broken or missing will simply need another PR to address it,
and we can get it in. I actually don't think this should be too bad. Again,
thank you to Shane, Tibor, and Tamas for breaking this down. Just be
careful in the future when you're collaborating on something of this size
that you either A) split off the work in fully functioning PRs, e.g.
refactoring work or library changes can very easily be submitted against
master without breaking anything or B) start with a feature branch and
discuss thread that lays out your plan for delivering the feature in that
FB.

Please review Apache's guide on consensus building, as well -
http://community.apache.org/committers/consensusBuilding.html

On Tue, Jun 11, 2019 at 12:01 PM Ryan Merriman  wrote:

> "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."
>
> I don't think this is going to work.  If changes are requested and applied
> in a different PR, how would you test the original PR?  What would you do
> if there were merge conflicts introduced between the current and final PR?
> What's the point of even having any PRs before the final one if they won't
> get committed or changed?  I don't see any way to avoid having to propagate
> changes through all subsequent PRs.
>
> On Wed, May 29, 2019 at 7:03 AM Tibor Meller 
> wrote:
>
> > 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 
> > 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
> > > 
> > > PR#2 METRON-2116: [UI] Removing redundant AppConfigService
> > > 
> > > PR#3 METRON-2117: [UI] Aligning models to grouping feature
> > > 
> > > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP
> > > 
> > > PR#5 METRON-2122: [UI] Fixing early app config access issue
> > > 
> > > PR#6 METRON-2124: [UI] Move status information and start/stop to the
> > > Aggregate level 
> > > PR#7 METRON-2125: [UI] Making changes visible in the parser list by
> > > marking changed items 
> > > PR#8 METRON-2131: Add NgRx and related dependencies
> > > 
> > > PR#9 METRON-2133: Add NgRx effects to communicate with the server
> > > 
> > > PR#10 METRON-2134: Add NgRx reducers to perform parser and group
> changes
> > > in the store 
> > > PR#11 METRON-2135: Add NgRx actions to trigger state changes
> > > 
> > > PR#12 METRON-2136: Add parser aggregation sidebar
> > > 
> > > PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx
> > > 
> > > PR#14 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

I must have missed this comment - I think we should be merging the PRs into
the feature branch as they are completed and approved because we need the
review history associated with them. This also ensures that any changes
requested get appropriately resolved in the final version of the feature
branch before it's pulled in. If you need to redo the final composite PR,
that's probably fine. Merging changes upstream should be a non-conflict
operation using "git merge" unless you're modifying the same things
multiple times.

On Thu, May 23, 2019 at 3:45 AM Tibor Meller  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.miklav...@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 
> > 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 
> > > 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.miklav...@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.ard...@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 
> > > > wrote:
> > > > >
> > > > > > Here's the process we've gone through in order to implement the
> > > > feature.
> > > > > >
> > > > > 

Re: [DISCUSS] Parser Aggregation in Management UI

[Ryan Merriman]

"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."

I don't think this is going to work.  If changes are requested and applied
in a different PR, how would you test the original PR?  What would you do
if there were merge conflicts introduced between the current and final PR?
What's the point of even having any PRs before the final one if they won't
get committed or changed?  I don't see any way to avoid having to propagate
changes through all subsequent PRs.

On Wed, May 29, 2019 at 7:03 AM Tibor Meller  wrote:

> 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 
> 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
> > 
> > PR#2 METRON-2116: [UI] Removing redundant AppConfigService
> > 
> > PR#3 METRON-2117: [UI] Aligning models to grouping feature
> > 
> > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP
> > 
> > PR#5 METRON-2122: [UI] Fixing early app config access issue
> > 
> > PR#6 METRON-2124: [UI] Move status information and start/stop to the
> > Aggregate level 
> > PR#7 METRON-2125: [UI] Making changes visible in the parser list by
> > marking changed items 
> > PR#8 METRON-2131: Add NgRx and related dependencies
> > 
> > PR#9 METRON-2133: Add NgRx effects to communicate with the server
> > 
> > PR#10 METRON-2134: Add NgRx reducers to perform parser and group changes
> > in the store 
> > PR#11 METRON-2135: Add NgRx actions to trigger state changes
> > 
> > PR#12 METRON-2136: Add parser aggregation sidebar
> > 
> > PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx
> > 
> > PR#14 METRON-2138: Code clean up
> > 
> > PR#15 METRON-2139: Refactoring sensor-parser-config.component and wire
> > NgRx 
> >
> > Thanks,
> > Tibor
> >
> >
> > On Thu, May 23, 2019 at 11:45 AM Tibor Meller 
> > 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.miklav...@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 
> >>> 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 

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

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  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
> 
> PR#2 METRON-2116: [UI] Removing redundant AppConfigService
> 
> PR#3 METRON-2117: [UI] Aligning models to grouping feature
> 
> PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP
> 
> PR#5 METRON-2122: [UI] Fixing early app config access issue
> 
> PR#6 METRON-2124: [UI] Move status information and start/stop to the
> Aggregate level 
> PR#7 METRON-2125: [UI] Making changes visible in the parser list by
> marking changed items 
> PR#8 METRON-2131: Add NgRx and related dependencies
> 
> PR#9 METRON-2133: Add NgRx effects to communicate with the server
> 
> PR#10 METRON-2134: Add NgRx reducers to perform parser and group changes
> in the store 
> PR#11 METRON-2135: Add NgRx actions to trigger state changes
> 
> PR#12 METRON-2136: Add parser aggregation sidebar
> 
> PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx
> 
> PR#14 METRON-2138: Code clean up
> 
> PR#15 METRON-2139: Refactoring sensor-parser-config.component and wire
> NgRx 
>
> Thanks,
> Tibor
>
>
> On Thu, May 23, 2019 at 11:45 AM Tibor Meller 
> 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.miklav...@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 
>>> 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 

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

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

PR#2 METRON-2116: [UI] Removing redundant AppConfigService

PR#3 METRON-2117: [UI] Aligning models to grouping feature

PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP

PR#5 METRON-2122: [UI] Fixing early app config access issue

PR#6 METRON-2124: [UI] Move status information and start/stop to the
Aggregate level 
PR#7 METRON-2125: [UI] Making changes visible in the parser list by marking
changed items 
PR#8 METRON-2131: Add NgRx and related dependencies

PR#9 METRON-2133: Add NgRx effects to communicate with the server

PR#10 METRON-2134: Add NgRx reducers to perform parser and group changes in
the store 
PR#11 METRON-2135: Add NgRx actions to trigger state changes

PR#12 METRON-2136: Add parser aggregation sidebar

PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx

PR#14 METRON-2138: Code clean up

PR#15 METRON-2139: Refactoring sensor-parser-config.component and wire NgRx


Thanks,
Tibor


On Thu, May 23, 2019 at 11:45 AM Tibor Meller 
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.miklav...@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 
>> 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
>> > - 

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

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.miklav...@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 
> 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 
> > 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.miklav...@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 
> > > 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 
> > > 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
> > > > > 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

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  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 
> 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.miklav...@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 
> > 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 
> > 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 

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

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  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.miklav...@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 
> 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 
> 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 <
> > > 

Re: [DISCUSS] Parser Aggregation in Management UI

[Otto Fowler]

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.miklav...@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 
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  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.miklav...@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  wrote:
> > >
> > > > Have you considered creating a feature branch for the effort? This
> > would
> > > > allow you to break the effort 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

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 
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  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.miklav...@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  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
> > > > 

Re: [DISCUSS] Parser Aggregation in Management UI

[Shane Ardell]

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  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.miklav...@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  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 
> > > 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. 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

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  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 
> 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 
> > 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.miklav...@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.ard...@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
> > > > >
> > > > 

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

Ok, thanks for the clarification.

On Fri, May 3, 2019 at 5:49 AM Shane Ardell 
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.miklav...@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 
> > 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
> > >
> >
>

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

Thanks Nick! That actually sounds promising.
Ryan are you ok with that if we open multiple smaller PR against the
Feature Branch you planning to create?


On Mon, May 6, 2019 at 4:19 PM Nick Allen  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 
> 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 
> > 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.miklav...@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.ard...@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 

Re: [DISCUSS] Parser Aggregation in Management UI

[Nick Allen]

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  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 
> 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.miklav...@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  >
> > > 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

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

@Otto: I responded to your questions in a few Jira comment.

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

Re: [DISCUSS] Parser Aggregation in Management UI

[Tibor Meller]

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 
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.miklav...@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 
> > 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
> > >
> >
>

Re: [DISCUSS] Parser Aggregation in Management UI

[Shane Ardell]

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.miklav...@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 
> 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
> >
>

Re: [DISCUSS] Parser Aggregation in Management UI

[Michael Miklavcic]

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 
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
>

Re: [DISCUSS] Parser Aggregation in Management UI

[Otto Fowler]

I have commented the jira.




On May 2, 2019 at 14:22:41, Shane Ardell (shane.m.ard...@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