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