metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Miklavcic <michael.miklav...@gmail.com>
Subject Re: [DISCUSS] Full-dev role in PR testign
Date Fri, 03 May 2019 16:37:50 GMT
That's awesome, Nick! Looking forward to seeing how this works out.

On Fri, May 3, 2019 at 10:06 AM Nick Allen <nick@nickallen.org> wrote:

> I'm exploring the use of TestContainers right now as part of the HDP 3.1
> effort.  Still exploring feasibility, but it is looking promising.
>
> On Fri, May 3, 2019 at 10:46 AM Justin Leet <justinjleet@gmail.com> wrote:
>
> > I think everything Casey mentioned is a good call-out as things start to
> > build into specifics. I definitely agree it's a very nontrivial amount of
> > work, but that lowering the barrier of entry to a lot of PRs eases the
> > burden on both new and existing contributors by a substantial amount.
> >
> > @Mike,
> > As a heads up, I (super briefly) looked into the Docker stuff a bit, and
> > the extension idea may not work with the Docker stuff to the extent we
> want
> > (at least without us doing some additional work ourselves).  It seems
> like
> > at least what I linked earlier and some other stuff actually provide
> direct
> > annotations rather than plugging directly into the same extensions idea.
> >
> > Before we dive into it too much, it might be worth playing around with it
> > more and coming back to the group with a couple options. If you're
> > interested in looking into it, I *suspect* it'll boil down to a couple
> > options
> > * Use Docker with something like the above link or testcontainers
> > <https://www.testcontainers.org/>. It's possible the Docker stuff ends
> up
> > being lightweight / fast enough to use on at least a per class basis like
> > we do now, rather than trying to generalize across all tests immediately.
> > * Roll our own code to have more fine-grained control over the Docker
> > lifecycle and which components need to be spun up for extensions.
> > * Figure out how to make the prior options play nice
> > * Other
> >
> > I'll probably dig a bit on my own, but I'm not sure how much focused time
> > I'll be able to put into it in the absolute immediate term.  I can
> probably
> > whip up a quick demo of the extensions stuff over the next week or so in
> a
> > one-off project to give a bit of a demo and maybe help out with some of
> > experimentation. Feel free to reach out if there's anything in particular
> > that would be helpful to look into.
> >
> > The extensions stuff does have a lot of benefits, but I had less
> components
> > to work with and didn't have the same classpath worries that made real
> > instances (i.e. Docker) more attractive. It was sufficient for our
> > purposes, but there might have to be compromises here. We depend on a lot
> > more of the Hadoop stack.
> >
> > Migrating to JUnit 5 in general *should* be pretty easy. I don't think we
> > really use any of the stuff that migrated in odd ways, so it should
> mostly
> > just be updating annotations and imports (@Before to @BeforeEach, etc.).
> > I'm sure this glosses over at least a few gotchas, though.
> >
> > On Fri, May 3, 2019 at 10:09 AM Michael Miklavcic <
> > michael.miklavcic@gmail.com> wrote:
> >
> > > I didn't get a chance to say so earlier, but Justin, I also like the
> > JUnit
> > > 5 extension suggestion. I've gone through some en-masse changes before,
> > > e.g. standardizing the log4j construction idiom, and it honestly wasn't
> > too
> > > bad. Just a thought, it might make sense to kick this off by upgrading
> > > overall JUnit 4 to 5 across the code base, and then diving into some of
> > the
> > > more 5-specific changes you're recommending as needed. I created this
> > Jira
> > > a bit ago - https://issues.apache.org/jira/browse/METRON-2037. That
> was
> > to
> > > upgrade to 4.13, but we might be able to kill 2 birds with one stone if
> > we
> > > go to JUnit 5. I'm volunteering to look into this and/or see the work
> > > through to completion. What do you think?
> > >
> > > > - debuggability (right now we run the tests in the same JVM and
> setting
> > >    breakpoints is trivial, even in the innards of Hadoop.  This is very
> > >    valuable for figuring out what's going wrong and we'll need SOME
> > > solution
> > >    for it)
> > >
> > > Yeah Casey, I remember this from the last time we discussed it. That's
> > the
> > > most import issue to be sure we have a handle on, imo. We'll need to
> > figure
> > > out remote debugging in Docker containers. Not to mention, the
> execution
> > > path becomes a bit more spread out when we're running multiple
> components
> > > as nature intended across multiple processes.
> > >
> > >
> > >
> > > On Fri, May 3, 2019 at 7:14 AM Casey Stella <cestella@gmail.com>
> wrote:
> > >
> > > > I just want to chime in and say I'm STRONGLY in favor of a
> docker-based
> > > > approach to testing (I specifically like the JUnit 5 extensions
> > > > suggestion).  I think that forcing a full-dev evaluation for every
> > small
> > > PR
> > > > is a barrier to entry that I'd like to overcome.  I also think that
> > this
> > > is
> > > > going to not be trivial.
> > > >
> > > > There will be weirdness/drama with:
> > > >
> > > >    - cleanup
> > > >    - setup in situations where multi-components are used
> > > >    - debuggability (right now we run the tests in the same JVM and
> > > setting
> > > >    breakpoints is trivial, even in the innards of Hadoop.  This is
> very
> > > >    valuable for figuring out what's going wrong and we'll need SOME
> > > > solution
> > > >    for it)
> > > >    - possible resource limitations in travis for running tests with
> > > >    multiple components
> > > >
> > > > Even so, with ALL of that being said, I still think the value
> outweighs
> > > the
> > > > difficulty by a factor of 10.  Being able to be confident after a
> > travis
> > > > run that people aren't introducing subtle classpath or
> cross-component
> > > > interaction issues would open up 80% of the class of PRs that don't
> > > require
> > > > full-dev review.  That being said, I still don't think it's 100%.
> > > > Specifically, PRs which can credibly be argued that they touch
> > > installation
> > > > pathways would still need to be verified in full-dev as it's the only
> > > path
> > > > to validating that (otherwise we would be regressing in test
> coverage).
> > > >
> > > > On Wed, May 1, 2019 at 9:33 PM Justin Leet <justinjleet@gmail.com>
> > > wrote:
> > > >
> > > > > >
> > > > > > My impression is that this is already the status quo. But, if
we
> > > think
> > > > we
> > > > > > need to be more clear on this, let's put up a vote to change
the
> > > coding
> > > > > > guidelines and PR checklist. I've done this many times in the
> past,
> > > the
> > > > > > most obvious instances are when I've made doc changes or unit
> test
> > > > > > modifications because those will not impact full dev. I will
own
> > this
> > > > > item.
> > > > > > I think it can probably get rolled in with my dev guideline
> changes
> > > for
> > > > > > architecture diagrams.
> > > > >
> > > > >
> > > > > For completeness in our PR checklist: "- [ ] Have you verified the
> > > basic
> > > > > functionality of the build by building and running locally with
> > Vagrant
> > > > > full-dev environment or the equivalent?"  In practice, you're
> right,
> > > but
> > > > > any newer contributors aren't necessarily going to know this.
> > > > >
> > > > > 1. I think we should create Jiras with the end deliverable being
> that
> > > our
> > > > > > private vs public API endpoints are clearly delineated. From
> there,
> > > we
> > > > > > create another round of javadoc - for the public APIs let the
> > > javadocs
> > > > > rain
> > > > > > from the heavens to your heart's content. It's for public
> > consumption
> > > > and
> > > > > > should assist end users. See Mockito, for example -
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://static.javadoc.io/org.mockito/mockito-core/2.27.0/org/mockito/Mockito.html
> > > > > > .
> > > > > > For developer docs, I'm of the *extremely strong* opinion that
> this
> > > > > should
> > > > > > be limited. Emphasize module, package, class, and method naming
> > > > > conventions
> > > > > > over all else. If it doesn't make sense just reading the code,
> > take a
> > > > > > minute to summarize what you're doing and consider refactoring.
> For
> > > > > > legitimately more complex and necessary code passages, add a
> note.
> > > For
> > > > > > multi-class interactions that provide a larger story arc, add
dev
> > > docs
> > > > to
> > > > > > the relevant READMEs. Our use of Zookeeper Curator and its
> > > interaction
> > > > > with
> > > > > > our topology config loading is a perfect example of a feature
> that
> > > > would
> > > > > > fit this need.
> > > > > >
> > > > > 2. I'm an immediate -1 on any documentation that looks like " /**
> > Open
> > > > the
> > > > > > car door **/ public void openCarDoor() {...}" :-). The code
> speaks
> > > for
> > > > > > itself.
> > > > > > 3. Publish javadocs for public APIs, not our internal dev APIs.
> Let
> > > > your
> > > > > > fav IDE fill in the gaps for devs.
> > > > >
> > > > >
> > > > > I'm +1 on delineating public vs private APIs like you've outlined
> > > > there.  I
> > > > > think our dev stuff is *better* than our general usage guides, but
> > > > there's
> > > > > room for improvement. I'm fairly agnostic on the dev docs because
> to
> > be
> > > > > honest, a ton of our older code is not at all self explanatory, and
> > to
> > > be
> > > > > blunt refactoring a lot of it is a substantial lift (as we've all
> > seen
> > > > > multiple times trying to refactor it).  If this were greenfield,
> I'd
> > be
> > > > in
> > > > > much stronger agreement with you, but I suspect in practice
> there's a
> > > lot
> > > > > of stuff nobody's going to refactor for awhile.
> > > > >
> > > > >
> > > > > > Full dev until we vote to replace the existing setup and can
be
> > > > confident
> > > > > > that the new approach 1. is stable, 2. takes <= the amount
of
> time
> > to
> > > > > > complete as full dev. I am +1 for migrating towards this approach
> > and
> > > > > think
> > > > > > we should do so when it's dialed in.
> > > > >
> > > > >
> > > > > Great, I look forward to that getting in.
> > > > >
> > > > > Justin, what are your thoughts on leveraging this approach along
> with
> > > > > > long-lived Docker containers?
> > > > > >
> > > > >
> > > > > Apparently, there's actually an extension for running Docker
> > > containers,
> > > > > see  https://faustxvi.github.io/junit5-docker/.  My main
> hesitation
> > > > there
> > > > > is more around how much effort to migrate it is. I think that's
> > almost
> > > > > certainly a cleaner long term solution, but I suspect the 80%
> > solution
> > > of
> > > > > migrating what we have *might* be easier.  There might also be ways
> > of
> > > > just
> > > > > leveraging this by moving stuff over to failsafe from surefire,
> but I
> > > > > really don't know enough about it.
> > > > >
> > > > >  Clean/reset component state after test method and/or test class
> > > > > >    completion
> > > > >
> > > > >
> > > > > Probably doable regardless of approach, but things can take
> > nontrivial
> > > > > amounts of time to clean up (e.g. topic deletion or clearing). I
> > > believe
> > > > we
> > > > > do this right now, so I'd expect to continue with that.
> > > > >
> > > > >
> > > > > On Wed, May 1, 2019 at 8:48 PM Michael Miklavcic <
> > > > > michael.miklavcic@gmail.com> wrote:
> > > > >
> > > > > > Justin, what are your thoughts on leveraging this approach along
> > with
> > > > > > long-lived Docker containers? I think the lifecycle would look
> > like:
> > > > > >
> > > > > >    1. I need components A, B, C
> > > > > >    2. If not started, start A, B, C
> > > > > >    3. If started, clean/reset it
> > > > > >    4. Setup pre-test state
> > > > > >    5. Run test(s)
> > > > > >    6. Clean/reset component state after test method and/or test
> > class
> > > > > >    completion (we may consider nuking this step - I assisted
in
> > > > designing
> > > > > > an
> > > > > >    acceptance testing framework in the past where we handled
> > cleanup
> > > on
> > > > > the
> > > > > >    front end. This meant that remediation for failed tests became
> > > > simpler
> > > > > >    because I still had the state from the failed test run)
> > > > > >
> > > > > > Stopping/removing the containers could be a manual process or
> just
> > a
> > > > > simple
> > > > > > script in the project root. We could also add a special module
> that
> > > > runs
> > > > > > last that handles shutting down containers, if desired.
> > > > > >
> > > > > > I know this has been a perennial thread for us. I can dig up
the
> > > > original
> > > > > > discuss threads on this as well if folks think they're still
> > > pertinent,
> > > > > but
> > > > > > I think we're pretty far removed now from that original point
in
> > time
> > > > and
> > > > > > should just move forward with fresh perspectives.
> > > > > >
> > > > > > On Wed, May 1, 2019 at 9:21 AM Justin Leet <
> justinjleet@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > > Re: the integration testing point above, a strategy I used
> > recently
> > > > to
> > > > > > > alleviate a similar problem was to exploit JUnit 5's
> > extensions.  I
> > > > > > haven't
> > > > > > > played with this at all in Metron's code, so 1) Take this
with
> a
> > > > > several
> > > > > > > grains of salt and 2) Feel encouraged to point out anything
> > > > > > > wrong/problematic/ludicrous.  My use case was pretty tame
and
> > easy
> > > to
> > > > > > > implement.
> > > > > > >
> > > > > > > Essentially, you can set up an extension backed by a singleton
> > > > cluster
> > > > > > (in
> > > > > > > the case I was doing, I had two extensions, one for a Kafka
> > cluster
> > > > and
> > > > > > one
> > > > > > > for MiniDfs).  The backing clusters expose some methods
(i.e.
> > > create
> > > > > > topic,
> > > > > > > get brokers, get file system, etc.), which lets the tests
> classes
> > > > setup
> > > > > > > whatever they need. Classes that need them just use an
> annotation
> > > and
> > > > > can
> > > > > > > be setup under the hood to init only when tests in the
current
> > run
> > > > > suite
> > > > > > > actually use them and spin down after all are done. This
saved
> ~1
> > > min
> > > > > on
> > > > > > a
> > > > > > > 4 minute build.  It ends up being a pretty clean way to
use
> them,
> > > > > > although
> > > > > > > we might need to be a bit more sophisticated, since my
case was
> > > less
> > > > > > > complicated. The main messiness is that this necessarily
> invites
> > > > tests
> > > > > to
> > > > > > > interfere with each other (i.e. if multiple tests use the
kafka
> > > topic
> > > > > > > "test", problems will be involved). We might be able to
improve
> > on
> > > > > this.
> > > > > > >
> > > > > > > We could likely do something similar with our
> InMemoryComponents.
> > > > Right
> > > > > > now
> > > > > > > these are spun up on a per-test basis, rather than the
overall
> > > suite.
> > > > > > This
> > > > > > > involves some setup in any class that wants them, just
to be
> able
> > > to
> > > > > get
> > > > > > a
> > > > > > > cluster.  There's also some definition of spin up order
and so
> > on,
> > > > > which
> > > > > > is
> > > > > > > already taken care of with the extensions (declaration
order).
> > The
> > > > > other
> > > > > > > catch in our case is that we have way more code covered
by
> JUnit
> > 4,
> > > > > which
> > > > > > > doesn't have the same capabilities.  That migration doesn't
> > > > necessarily
> > > > > > > have to be done all at once, but it is a potential substantial
> > pain
> > > > > > point.
> > > > > > >
> > > > > > > Basically, I'd hope to push management of our abstraction
> further
> > > > back
> > > > > > into
> > > > > > > actual JUnit so they're get more reuse across runs and
it's
> > easily
> > > > > > > understood what a test needs and uses right up front. 
In our
> > case,
> > > > the
> > > > > > > InMemoryComponents likely map to extensions.
> > > > > > >
> > > > > > > If we did something like this, it potentially solves a
few
> > problems
> > > > > > > * Makes it easy to build an integration test that says
"Give me
> > > these
> > > > > > > components".
> > > > > > > * Keeps it alive across any test that needs them
> > > > > > > * Only spins it up if there are tests running that do need
> them.
> > > Only
> > > > > > spins
> > > > > > > up the necessary components.
> > > > > > > * Lower our build times.
> > > > > > > * Interaction with components remains primarily through
the
> same
> > > > > methods
> > > > > > > you'd normally interact (you can build producers/consumers,
or
> > > > > whatever).
> > > > > > > * IMO, easier to explain and have people use.  I've had
a
> couple
> > > > people
> > > > > > > pretty easily pick up on it.
> > > > > > >
> > > > > > > I can't share the code, but essentially it looks like (And
I'm
> > sure
> > > > > email
> > > > > > > will butcher this, so I'm sorry in advance):
> > > > > > > @ExtendWith({<ExtensionOne.class, ExtensionTwo.class,
...})
> > > > > > > ...
> > > > > > > @BeforeAll
> > > > > > > public static void beforeAll() {
> > > > > > >    // Test specific setup, similar to before. But without
the
> > > cluster
> > > > > > > creation work.
> > > > > > > }
> > > > > > >
> > > > > > > Everything else gets handled under the covers, with the
caveat
> > that
> > > > > what
> > > > > > I
> > > > > > > needed to do was less involved than some of our tests,
so there
> > may
> > > > be
> > > > > > some
> > > > > > > experimenting.
> > > > > > >
> > > > > > > On Wed, May 1, 2019 at 10:59 AM Justin Leet <
> > justinjleet@gmail.com
> > > >
> > > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I wanted to start a discussion on something near and
dear to
> > all
> > > of
> > > > > our
> > > > > > > > hearts: The role of full-dev in our testing cycle.
> > > > > > > >
> > > > > > > > Right now, we require all PRs to have spun up the
full-dev
> > > > > environment
> > > > > > > and
> > > > > > > > make sure that things flow through properly. In some
cases,
> > this
> > > > is a
> > > > > > > > necessity, and in other cases, it's a fairly large
burden on
> > > > current
> > > > > > and
> > > > > > > > potential contributors.
> > > > > > > >
> > > > > > > > So what do we need to do to reduce our dependence
on full dev
> > and
> > > > > > > increase
> > > > > > > > our confidence in our CI process?
> > > > > > > >
> > > > > > > > My initial thoughts on this encompass a few things
> > > > > > > > * Increasing our ability to easily write automated
tests. In
> > > > > > particular,
> > > > > > > I
> > > > > > > > think our integration testing is fairly hard and has
some
> > > > structural
> > > > > > > issues
> > > > > > > > (e.g. our tests spin up components per Test class,
not for
> the
> > > > > overall
> > > > > > > test
> > > > > > > > suite being run, which also increases build times,
etc.). How
> > do
> > > we
> > > > > > make
> > > > > > > > this easier and have less boilerplate?  I have one
potential
> > idea
> > > > > I'll
> > > > > > > > reply to this thread with.
> > > > > > > > * Unit tests in general. I'd argue that any area we
thing
> need
> > > > > full-dev
> > > > > > > > spun up to be confident in needs more testing. Does
anyone
> have
> > > any
> > > > > > > > opinions on which areas we have the least confidence
in?
> Which
> > > > areas
> > > > > > of
> > > > > > > > code are people afraid to touch?
> > > > > > > > * Our general procedures. Should we not be requiring
full-dev
> > on
> > > > all
> > > > > > PRs,
> > > > > > > > but maybe just asking for a justification why not
(e.g. "I
> > don't
> > > > need
> > > > > > > > full-dev for this, because I added unit tests here
and here
> and
> > > > > > > integration
> > > > > > > > tests for the these components?"). And then a reviewer
can
> > > request
> > > > a
> > > > > > > > full-dev spin up if needed?  Could we stage this rollout
> (e.g.
> > > > > > > > metron-platform modules require it, but others don't,
etc.?)
> > > > This'll
> > > > > > add
> > > > > > > > more pressure onto the release phase, so maybe that
involves
> > > > fleshing
> > > > > > > out a
> > > > > > > > checklist of critical path items to check.
> > > > > > > > * Do we need to improve our docs? Publish Javadocs?
After
> all,
> > if
> > > > > docs
> > > > > > > are
> > > > > > > > better, hopefully we'd see less issues introduced
and be more
> > > > > confident
> > > > > > > in
> > > > > > > > changes coming in.
> > > > > > > > * What environment do we even want testing to occur
in?
> > > > > > > > https://github.com/apache/metron/pull/1261 has seen
a lot of
> > > work,
> > > > > and
> > > > > > > > getting it across the finish line may help make the
dev
> > > environment
> > > > > > less
> > > > > > > > onerous, even if still needed.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message