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 <tibor.mel...@gmail.com> 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 feature > <https://github.com/apache/metron/pull/1412> > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP > <https://github.com/apache/metron/pull/1414> > PR#5 METRON-2122: [UI] Fixing early app config access issue > <https://github.com/apache/metron/pull/1415> > PR#6 METRON-2124: [UI] Move status information and start/stop to the > Aggregate level <https://github.com/apache/metron/pull/1418> > PR#7 METRON-2125: [UI] Making changes visible in the parser list by > marking changed items <https://github.com/apache/metron/pull/1422> > PR#8 METRON-2131: Add NgRx and related dependencies > <https://github.com/apache/metron/pull/1423> > PR#9 METRON-2133: Add NgRx effects to communicate with the server > <https://github.com/apache/metron/pull/1424> > PR#10 METRON-2134: Add NgRx reducers to perform parser and group changes > in the store <https://github.com/apache/metron/pull/1425> > PR#11 METRON-2135: Add NgRx actions to trigger state changes > <https://github.com/apache/metron/pull/1426> > PR#12 METRON-2136: Add parser aggregation sidebar > <https://github.com/apache/metron/pull/1427> > PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx > <https://github.com/apache/metron/pull/1428> > PR#14 METRON-2138: Code clean up > <https://github.com/apache/metron/pull/1429> > PR#15 METRON-2139: Refactoring sensor-parser-config.component and wire > NgRx <https://github.com/apache/metron/pull/1430> > > Thanks, > Tibor > > > On Thu, May 23, 2019 at 11:45 AM Tibor Meller <tibor.mel...@gmail.com> > 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 <tibor.mel...@gmail.com> >>> wrote: >>> >>> > I would like to describe quickly *our approach to breaking down Parser >>> > Aggregation PR for smaller chunks* >>> > >>> > *1. we squashed the commits in the original development branch* >>> > - when we started to open smaller PRs from the commits from the >>> original >>> > branch, we found ourself opening PRs out of historical states of the >>> code >>> > instead of the final one >>> > - none of those states of development are worth (or make sense) to be >>> > reviewed (initial phases of development are included in the original >>> commit >>> > history, multiple iterations of refactoring, etc.) >>> > - while the actual development history was irrelevant, the attribution >>> > aspect of it was still important >>> > *2. we divided** the changes by the original authors* >>> > - the original contributors were sardell, ruffle1986 and tiborm >>> > - we isolated the changes that belong to each individual contributor >>> > *3. each of us identified smaller but belonging changesets * >>> > - with this, we ended up opening 5 PRs from tiborm, 3 from sardell and >>> 6 >>> > from ruffle1986 >>> > - each of these are smaller than 500 lines of changes, which makes the >>> task >>> > of reviewing easier >>> > - they have their own context and purpose described by the PR and the >>> > related Jira ticket >>> > *4. Each PR introduces a single new commit which is meant to be >>> reviewed* >>> > - with this we were able to open PRs on top of each others work, but >>> the >>> > reviewer is still able to identify what changes were introduced and >>> > described by the pr simply by focusing on the last commit >>> > - the commit introduced by the PR has the same commit message as the >>> title >>> > of the PR to make it easier to find >>> > *5. Only the last PR is meant to be merged into the feature branch* >>> > - the last PR also introduces a single new commit to being reviewed >>> > - this contains all the commits from the previous PRs that belong to >>> parser >>> > aggregation >>> > - it builds fine in Travis >>> > - it's fully functional and ready to being tested against full dev >>> > - If we only merge the last PR, we don't have to rebase and recreate >>> all of >>> > our PRs due to merge conflicts that will result from to conflicting >>> > histories (which is common in feature branch work) >>> > >>> > Once all the Pull Requests are open, I will submit a list of all of >>> them to >>> > this discussion thread. >>> > >>> > On Wed, May 8, 2019 at 3:58 PM Otto Fowler <ottobackwa...@gmail.com> >>> > wrote: >>> > >>> > > You need to be a committer, that is all I think. >>> > > I would not use the github UI for it though, I do it through the cli >>> > > >>> > > >>> > > >>> > > On May 8, 2019 at 09:45:24, Michael Miklavcic ( >>> > michael.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 <ftamas.m...@gmail.com> >>> > > wrote: >>> > > > >>> > > > > Here's the process we've gone through in order to implement the >>> > > feature. >>> > > > > >>> > > > > At the beginning we had some bootstrap work like creating a mock >>> API >>> > > > > (written in NodeJS) because we were a few steps ahead the backend >>> > part. >>> > > > But >>> > > > > this is in a totally different repository so it doesn't really >>> count. >>> > > We >>> > > > > also had to wire NgRX, our chosen 3rd party that supports the >>> flux >>> > flow >>> > > > to >>> > > > > get a better state management. When we were ready to kick off >>> > > > implementing >>> > > > > the business logic in, we splited up the work by subfeatures like >>> > drag >>> > > > and >>> > > > > dropping table rows. At this point, we created a POC without NgRX >>> > just >>> > > to >>> > > > > let you have the feeling of how it works in real life. Later on, >>> > after >>> > > > > introducing NgRX, we had to refactor it a little bit obviously to >>> > make >>> > > it >>> > > > > compatible with NgRX. There were other subfeatures like creating >>> and >>> > > > > editing groups in a floating pane on the right side of the >>> window. >>> > > > > When the real backend API was ready we made the necessary >>> changes and >>> > > > > tested whether it worked how it was expected. There were a few >>> > > difference >>> > > > > between how we originally planned the API and the current >>> > > implementation >>> > > > so >>> > > > > we had to adapt it accordingly. While we were implementing the >>> > > features, >>> > > > we >>> > > > > wrote the unit tests simultaneously. The latest task on the >>> feature >>> > was >>> > > > > restricting the user from aggregating parsers together. >>> > > > > >>> > > > > As a first iteration, we've decided to put the restriction in >>> because >>> > > it >>> > > > > requires a bigger effort on the backend to deal with that. In my >>> > > opinion, >>> > > > > we should get rid of the restriction because it's not intuitive >>> and >>> > > very >>> > > > > inconvenient. In my opinion, we should let the users to >>> aggregate the >>> > > > > running parsers together and do the job to handle this edge case >>> on >>> > the >>> > > > > backend accordingly. >>> > > > > >>> > > > > What do you think, guys? >>> > > > > >>> > > > > Hope this helps. >>> > > > > >>> > > > > Tamas >>> > > > > >>> > > > > On Tue, May 7, 2019 at 4:34 PM Michael Miklavcic < >>> > > > > michael.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 <n...@nickallen.org> >>> > > wrote: >>> > > > > > >>> > > > > > > Have you considered creating a feature branch for the effort? >>> > This >>> > > > > would >>> > > > > > > allow you to break the effort into chunks, where the result >>> of >>> > each >>> > > > PR >>> > > > > > may >>> > > > > > > not be a fully working "master-ready" result. >>> > > > > > > >>> > > > > > > I am sure you guys tackled the work in chunks when >>> developing it, >>> > > so >>> > > > > > > consider just replaying those chunks onto the feature branch >>> as >>> > > > > separate >>> > > > > > > PRs. >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > On Mon, May 6, 2019 at 5:24 AM Tibor Meller < >>> > > tibor.mel...@gmail.com> >>> > > >>> > > > > > > wrote: >>> > > > > > > >>> > > > > > > > I wondered on the weekend how we could split that PR to >>> smaller >>> > > > > chunks. >>> > > > > > > > That PR is a result of almost 2 months of development and I >>> > don't >>> > > > see >>> > > > > > how >>> > > > > > > > to split that to multiple WORKING parts. It is as it is a >>> whole >>> > > > > working >>> > > > > > > > feature. If we split it by packages or files we could >>> provide >>> > > > smaller >>> > > > > > > > non-functional PR's, but can end up having a broken >>> Management >>> > UI >>> > > > > after >>> > > > > > > > having the 1st PR part merged into master. I don't think >>> that >>> > > would >>> > > > > be >>> > > > > > > > acceptable by the community (or even by me) so I would >>> like to >>> > > > > suggest >>> > > > > > > two >>> > > > > > > > other option to help review PR#1360. >>> > > > > > > > >>> > > > > > > > #1 We could extend that PR with our own author comments in >>> > > Github. >>> > > > > That >>> > > > > > > > would help following which code part belongs to where and >>> why >>> > it >>> > > > was >>> > > > > > > > necessary. >>> > > > > > > > #2 We can schedule an interactive code walkthrough call >>> with >>> > the >>> > > > ones >>> > > > > > who >>> > > > > > > > interested in reviewing or the particular changeset. >>> > > > > > > > >>> > > > > > > > Please share your thoughts on this! Which version would >>> support >>> > > you >>> > > > > the >>> > > > > > > > best? Or if you have any other idea let us know. >>> > > > > > > > >>> > > > > > > > PS: I think the size of our PR's depends on how small >>> > > independently >>> > > > > > > > deliverable changesets we can identify before we starting >>> to >>> > > > > implement >>> > > > > > a >>> > > > > > > > relatively big new feature. Unfortunately, we missed to do >>> that >>> > > > with >>> > > > > > this >>> > > > > > > > feature. >>> > > > > > > > >>> > > > > > > > On Fri, May 3, 2019 at 1:49 PM Shane Ardell < >>> > > > > shane.m.ard...@gmail.com> >>> > > > > > > > wrote: >>> > > > > > > > >>> > > > > > > > > NgRx was only used for the aggregation feature and >>> doesn't go >>> > > > > beyond >>> > > > > > > > that. >>> > > > > > > > > I think the way I worded that sentence may have caused >>> > > > confusion. I >>> > > > > > > just >>> > > > > > > > > meant we use it to manage more pieces of state within the >>> > > > > aggregation >>> > > > > > > > > feature than just previous and current state of grouped >>> > > parsers. >>> > > > > > > > > >>> > > > > > > > > On Fri, May 3, 2019 at 1:32 AM Michael Miklavcic < >>> > > > > > > > > michael.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 (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 >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> >>
