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