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 [email protected], 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.
---
