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