Re: [DISCUSS] Parser Aggregation in Management UI
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
@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
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 > > > > > >
