Concurrent tests hitting OOME, hangs, etc

[Kirk Lund <kl...@apache.org>]

The concurrent tests (tests using the ConcurrentTestRunner from
geode-concurrency-test) are currently blocking both of my PRs.

1st PR is upgrading Mockito -- several Concurrent Tests use Mockito but the
runner executes too many tests in parallel and Mockito can't keep up with
it -- they hit OOME and hang the PR job

2nd PR is changing Cache.close() to block -- same as above except that
Mockito isn't upgraded in this PR

Other devs have recommended that I move the concurrent tests to a new src
set for various reasons -- such as: these tests try very hard to use all
CPUs even when running in parallel and most of them take several minutes to
run, and now they're blocking changes like upgrading Mockito. But creating
a new src set and new CI job is also a huge headache and I really don't
want to do it.

Anyone else up for moving them to new src sets? Unfortunately, this alone
won't make the tests work with the newer versions of Mockito -- and guess
what, they only OOME in the cloud (not locally).

Should I disable these tests?

Should I remove the use of ConcurrentTestRunner from these tests
(converting them to simple junit tests)?

I spent some time studying ConcurrentTestRunner and I'm sorry, but I don't
really see the value in this runner, which is why I want to remove the use
of it and convert these tests to plain junit tests.

I also already tried changing the tests to use Mockito features stubOnly,
resetMocks, and clearInlineMocks -- these changes prevented OOME in
HARegionQueueIntegrationTest which was the only
non-ConcurrentTestRunner-test that had problems with upgrading Mockito.
Unfortunately these changes have done nothing to improve the running of
the ConcurrentTestRunner tests.

Related Mockito links:
* https://code.google.com/archive/p/mockito/issues/84
* https://github.com/mockito/mockito/issues/1825

-Kirk

Re: Concurrent tests hitting OOME, hangs, etc

[Kirk Lund <kl...@apache.org>]

AvailableConnectionManagerConcurrentTest is passing after changing it to
use plain java coded stubs (no Mockito) which required some changes to
Connection and PooledConnection (internal classes). The other
ConcurrentTests are passing on the Mockito upgrade PR branch.

Locally, AvailableConnectionManagerConcurrentTest was taking 10+ minutes to
run, while in the cloud it was causing IntegrationTest to hang and/or hit
OOME. After changing it to use plain java coded stubs it passes locally in
11 seconds.

The Cache.close() branch is unrelated. I mistook the 10+ minute run
of AvailableConnectionManagerConcurrentTest to indicate that it the
ConcurrentTest run a looong time even on develop (but they don't). The hang
on that branch is also in IntegrationTest but it's caused by reentrant
calls to Cache.close().

After Jake's PR is merged to develop, I'll rebase the Mockito upgrade on
develop -- so far it looks good after these latest changes
for AvailableConnectionManagerConcurrentTest.

On Thu, Apr 16, 2020 at 4:24 PM Dan Smith <ds...@pivotal.io> wrote:

> I like the idea of a separate concurrency test suite. We have other tests
> of concurrency that aren't using this runner that could also go there -
> maybe we could establish some good conventions and figure out how to give
> more CPU time to these tests. I'd actually like to see *more* tests of
> concurrency at a lower level, so we can catch race conditions sooner.
>
> > Should I disable these tests?
>
> I don't think we should be marking any tests as ignored. Let's figure out
> what is going on and make the appropriate fixes.
>
> Can you clarify why these tests are blocking your Cache.close PR? Also,
> which tests are getting an OOME with the mockito PR?
>
> For a little clarity on the ConcurrentTestRunner, it just runs a test with
> multiple threads some number of times. For example
>
> FilterProfileConcurrencyTest.serializationOfFilterProfileWithConcurrentUpdateShouldSucceed
> is testing a bug some of our customers hit where serializing an object that
> was being concurrently modified failed. I think there is definitely value
> in having those sorts of tests given the type of code we have - a lot of
> classes that are mutable and need to be thread safe.
>
> -Dan
>
>
> On Thu, Apr 16, 2020 at 2:47 PM Jacob Barrett <jb...@pivotal.io> wrote:
>
> >
> >
> > > On Apr 16, 2020, at 2:16 PM, Kirk Lund <kl...@apache.org> wrote:
> >
> > > Anyone else up for moving them to new src sets? Unfortunately, this
> alone
> > > won't make the tests work with the newer versions of Mockito -- and
> guess
> > > what, they only OOME in the cloud (not locally).
> >
> > I could take a stab at it since I split up all the test types originally.
> > I should be able to brush off those cobwebs.
> >
> > > Should I disable these tests?
> >
> > In an effort to get the Mockito stuff through, yes disable them.
> >
> > > Should I remove the use of ConcurrentTestRunner from these tests
> > > (converting them to simple junit tests)?
> > >
> > > I spent some time studying ConcurrentTestRunner and I'm sorry, but I
> > don't
> > > really see the value in this runner, which is why I want to remove the
> > use
> > > of it and convert these tests to plain junit tests.
> >
> > They have less value as unit tests since there are already unit tests
> that
> > cover them. It does raise a good questions about the value and
> > implementation of micro concurrent testing like this. I think isolating
> > them away is a good step before starting this conversation.
> >
> >
> >
>

Re: Concurrent tests hitting OOME, hangs, etc

[Dan Smith <ds...@pivotal.io>]

I like the idea of a separate concurrency test suite. We have other tests
of concurrency that aren't using this runner that could also go there -
maybe we could establish some good conventions and figure out how to give
more CPU time to these tests. I'd actually like to see *more* tests of
concurrency at a lower level, so we can catch race conditions sooner.

> Should I disable these tests?

I don't think we should be marking any tests as ignored. Let's figure out
what is going on and make the appropriate fixes.

Can you clarify why these tests are blocking your Cache.close PR? Also,
which tests are getting an OOME with the mockito PR?

For a little clarity on the ConcurrentTestRunner, it just runs a test with
multiple threads some number of times. For example
FilterProfileConcurrencyTest.serializationOfFilterProfileWithConcurrentUpdateShouldSucceed
is testing a bug some of our customers hit where serializing an object that
was being concurrently modified failed. I think there is definitely value
in having those sorts of tests given the type of code we have - a lot of
classes that are mutable and need to be thread safe.

-Dan


On Thu, Apr 16, 2020 at 2:47 PM Jacob Barrett <jb...@pivotal.io> wrote:

>
>
> > On Apr 16, 2020, at 2:16 PM, Kirk Lund <kl...@apache.org> wrote:
>
> > Anyone else up for moving them to new src sets? Unfortunately, this alone
> > won't make the tests work with the newer versions of Mockito -- and guess
> > what, they only OOME in the cloud (not locally).
>
> I could take a stab at it since I split up all the test types originally.
> I should be able to brush off those cobwebs.
>
> > Should I disable these tests?
>
> In an effort to get the Mockito stuff through, yes disable them.
>
> > Should I remove the use of ConcurrentTestRunner from these tests
> > (converting them to simple junit tests)?
> >
> > I spent some time studying ConcurrentTestRunner and I'm sorry, but I
> don't
> > really see the value in this runner, which is why I want to remove the
> use
> > of it and convert these tests to plain junit tests.
>
> They have less value as unit tests since there are already unit tests that
> cover them. It does raise a good questions about the value and
> implementation of micro concurrent testing like this. I think isolating
> them away is a good step before starting this conversation.
>
>
>

Re: Concurrent tests hitting OOME, hangs, etc

[Jacob Barrett <jb...@pivotal.io>]

> On Apr 16, 2020, at 2:16 PM, Kirk Lund <kl...@apache.org> wrote:

> Anyone else up for moving them to new src sets? Unfortunately, this alone
> won't make the tests work with the newer versions of Mockito -- and guess
> what, they only OOME in the cloud (not locally).

I could take a stab at it since I split up all the test types originally. I should be able to brush off those cobwebs.

> Should I disable these tests?

In an effort to get the Mockito stuff through, yes disable them.

> Should I remove the use of ConcurrentTestRunner from these tests
> (converting them to simple junit tests)?
> 
> I spent some time studying ConcurrentTestRunner and I'm sorry, but I don't
> really see the value in this runner, which is why I want to remove the use
> of it and convert these tests to plain junit tests.

They have less value as unit tests since there are already unit tests that cover them. It does raise a good questions about the value and implementation of micro concurrent testing like this. I think isolating them away is a good step before starting this conversation.