Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-14396: De-flake memory tests with new TestUtils.restrictMemory #12995

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

gharris1727
Copy link
Contributor

@gharris1727 gharris1727 commented Dec 15, 2022

Rather than relying on measuring Runtime::freeMemory() and calling System.gc(), tests which want to assert that there are no excessive allocations should execute in a restricted-memory environment.

In order to do this, add a new TestUtil method which intentionally consumes memory in a way that allows tests to assert that the code under test does not over-allocate or leak memory. This is a temporary effect on a single JVM, and is reverted as soon as the test code completes.

Apply this fix to MemoryRecordsBuilderTest::testBuffersDereferencedOnClose(), which now fails if the eponymous close() call is omitted.

Signed-off-by: Greg Harris greg.harris@aiven.io

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Rather than relying on measuring Runtime::freeMemory() and calling System.gc(),
tests which want to assert that there are to excessive allocations should execute in a restricted-memory environment.

In order to do this, add a new TestUtil method which intentionally consumes memory in a way that allows tests
to assert that the code under test does not over-allocate or leak memory.
This is a temporary effect on a single JVM, and is reverted as soon as the test code completes.

Apply this fix to MemoryRecordsBuilderTest::testBuffersDereferencedOnClose(),
which now fails if the eponymous close() call is omitted.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Contributor Author

Also something I noticed through removing the close() call: none and gzip each pass the test, despite never having close() called.
This appears to be because they allocate no or very little memory, and thus don't fail in the restricted memory environment.
However, all of the other compression methods seem to naively allocate a full-size buffer, which did cause their tests to fail.

@ijuma
Copy link
Contributor

ijuma commented Dec 15, 2022

The challenge is that the JVM continues to be reused for several tests afterwards. Once you get an OOM, it can leave lasting effects.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Contributor Author

@ijuma Thanks for pointing that out. I never noticed any effects of the OOMEs on the persistent JVM or other tests, but if I'm trying to de-flake the tests, introducing a new kind of flakiness that's even harder to debug is certainly not a good idea.

I've taken the high memory-pressure tests that I know of, and moved them from the test target to a new memoryIsolationTest target. This target will run the test classes in throw-away JVMs, so that any effects of the OOMEs will only affect that single run of that single test class.

This means that these specific tests (three once i'm finished) will no longer be run as part of test, but will still be executed by CI builders. I hope that this is an acceptable compromise, and will allow us to keep these memory pressure tests around.

Thanks!

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727 gharris1727 marked this pull request as ready for review January 6, 2023 19:34
@divijvaidya divijvaidya added the tests Test fixes (including flaky tests) label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Test fixes (including flaky tests)
Projects
None yet
3 participants