Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/774 @ottobackwards says: > would it be possible for you to help us with a comment as to your approach when you reviewed this functionality? It might help if you could summarize your feelings as well {with music} Oh spelunking we shall go, spelunking we shall go... :-) {with music} While reviewing the original incarnation of this, I tried to focus on the part most other Metron community members wouldn't be familiar with, the NiFi Bundle implementation, which I have some (small) experience with due to past work with NiFi. The review I did was an eyeball line-by-line code review. Of course actually running the code is also necessary, and at the time @mmiklavc was focusing on that side of things. Knowing that (a) the NiFi feature works in NiFi, and (b) Otto's intent was to change it as little as possible while porting it into Metron, I took an approach to reviewing it that focused on the **changes.** Specifically, as best I remember, I applied the following logic: * I pulled a copy of the version of NiFi that Otto used as a base * I pulled a copy of Otto's dev branch (submitted in PR#530) * I observed relationships between directory paths of corresponding files (code organization relationships) in the two code sets, including filename changes (which were mostly pattern-based) * I applied "diff -w" to corresponding file pairs * I reviewed THAT as the code deltas of interest, the way we would code review a simple PR based on master * I also reviewed in detail the small number of newly created files, and missing/deleted files, compared to the related NiFi sources * I mapped my questions and remarks back to the submitted code in Otto's PR#530, and made the usual comments in that PR * We interacted and followed up as usual. After many days and some untold number of comments, responses, and changes, I felt I understood and agreed with the state of the code, and gave it a +1 here: https://github.com/apache/metron/pull/530#issuecomment-322970038 , contingent on the practical trials being completed. Now, we've changed gears a bit. Faced with the difficulty of trying to review the offering all at once, whether in a PR or in a "feature branch" (turned out that wasn't much help after all), @ottobackwards and I proposed, and you'all accepted, the idea of breaking it up into five or so chunks. This was specified in my email to dev@metron.apache.org, 25 Sep 2017, Subject: Re: feature branch bumps (see http://mail-archives.apache.org/mod_mbox/metron-dev/201709.mbox/browser , Thread view, page 2, about 3/4 way down the page). It was specifically called out that for the first chunk, consisting of just the âbundleâ mechanism and the maven plugin, > "Review is acknowledged to be purely theoretical, and testing is based on the junit tests and integration tests. Weâll add a simple integration test with a synthetic test jar containing a trivial Class, unrelated to parsers, invoked in a test case via reflection (hence needing no glue code). After passing that level of review and testing, it would be committed, with an understanding that additional revisions might be required as the result of subsequent PR comments. So on the one hand, we agreed to a fairly light-weight review process, since by the nature of a piece-wise submission, the first part can't be tested on parsers. I encourage you to stick with that resolve. On the other hand, the simple integration test suggested does call for being able to see a "hello-world" Stellar command loaded and run from a bundle, which actually would be kinda exciting. @ottobackwards , can your test bundle be turned into a Stellar function, loaded, and invoked (by reflection so you don't need to worry about registration)? That would look more like an integration test than that bundle hanging out by itself.
---