Skip to content

KAFKA-7660: fix streams and Metrics memory leaks#5983

Merged
mjsax merged 2 commits intoapache:0.11.0from
vvcephei:0.11.0-memory-leaks
Dec 6, 2018
Merged

KAFKA-7660: fix streams and Metrics memory leaks#5983
mjsax merged 2 commits intoapache:0.11.0from
vvcephei:0.11.0-memory-leaks

Conversation

@vvcephei
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei commented Nov 30, 2018 •
edited
Loading

Backport two memory-leak fixes (#5974 and #5953) (see also 2.1: #5979, 2.0: #5980, 1.1: #5981, 1.0: #5982 )

It looks like we already had the parentSensors fix in 0.11.0 and lost it in 2.0. The change in
this PR just tidies it up a little for consistency with later branches.

Committer Checklist (excluded from commit message)

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

@vvcephei vvcephei mentioned this pull request Nov 30, 2018
Merged
3 tasks
@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Nov 30, 2018

@ijuma Again, sorry for the spam, but the 0.11.0 builder also looks misconfigured.
The JDK7 build fails with the same error as #5982 (comment) and #5981 (comment)

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 1, 2018

Retest this please

1 similar comment
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 2, 2018

Retest this please

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Dec 2, 2018

retest this please

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Dec 3, 2018

hmm. The java 7 build was aborted; not sure why.

The java 8 build failed because a test timed out: org.apache.kafka.streams.integration.QueryableStateIntegrationTest.concurrentAccesses. There were a bunch of connection exceptions in stdout.

I'll run it again to see what we see.

Retest this, please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 4, 2018

Checkstyle failed.

vvcephei and others added 2 commits December 4, 2018 10:28
@vvcephei
KAFKA-7660: tidy up parentSensors removal (#5953)
759fc48
@vvcephei
KAFKA-7660: Fix child sensor memory leak (#5974)
1578e3c 
A heap dump provided by Patrik Kleindl in https://issues.apache.org/jira/browse/KAFKA-7660 identifies the childrenSensors map in Metrics as keeping references to sensors alive after they have been removed.

This PR fixes it and adds a test to be sure.

Reviewers: Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Dec 4, 2018

Thanks, @mjsax; I've fixed it.

Awesome that it doesn't fail the build for that error :/

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 4, 2018

Java8 passed. Java7 failed with know flaky test.

Retest this please.

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Dec 5, 2018 •
edited
Loading

Java 8 failed in:

org.apache.kafka.streams.integration.QueryableStateIntegrationTest.concurrentAccesses

Error Message
java.lang.AssertionError: Key not found forest
Stacktrace
java.lang.AssertionError: Key not found forest
	at org.junit.Assert.fail(Assert.java:88)
	at org.apache.kafka.streams.integration.QueryableStateIntegrationTest.verifyGreaterOrEqual(QueryableStateIntegrationTest.java:888)
	at org.apache.kafka.streams.integration.QueryableStateIntegrationTest.concurrentAccesses(QueryableStateIntegrationTest.java:401)

Java 7 looks like the tests hung around the 30 minute mark, then the next message is that the test was aborted after 3 hours. Not sure what that's about.

Retest this, please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 6, 2018

Tests passed locally for me. Merging.

@mjsax mjsax merged commit df96b7a into apache:0.11.0 Dec 6, 2018
@vvcephei vvcephei deleted the 0.11.0-memory-leaks branch December 7, 2018 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vvcephei @mjsax @ijuma