-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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-9306: The consumer must close KafkaConsumerMetrics #7839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Colin for finding this. Left one comment.
clients/src/main/java/org/apache/kafka/clients/consumer/internals/KafkaConsumerMetrics.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing off from testing side of things. Verified that this change fixes the issue.
LGTM. Thanks @cmccabe for the fix! |
I pared this back to a more minimalistic fix for now, so that we can get something that can address the memory leak we spotted in 2.4. This will be easy to backport. I think we should do a better fix later which makes |
Seems the main problem here is the lambda inside |
I think I see what's going on. Both KafkaMbean mbean = removeAttribute(metric, mBeanName);
if (mbean != null) {
if (mbean.metrics.isEmpty()) {
unregister(mbean);
mbeans.remove(mBeanName);
} else
reregister(mbean);
} The This bug seems to be the result of a lot of general sloppiness in the metrics library and the jmx reporter. We should probably file a JIRA to revisit these apis to make it harder for us to make this kind of mistake. We can improve testing as well probably to check for dangling mbeans. |
@hachikuji : great analysis. I'm thinking we get this current PR in for future dot releases, and apply #7851 to trunk. Can I get an LGTM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the current patch. Can you add a test case? I think it would be straightforward to add an assertion to one of the test cases in KafkaConsumerTest
which verifies that all mbeans have been unregistered.
added a test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix.
Reviewers: Vikas Singh <vikas@confluent.io>, Jason Gustafson <jason@confluent.io>, Shailesh Panwar <spanwar@confluent.io> (cherry picked from commit 7e36865)
Backported to 2.4 in case we do a dot release on that branch. |
Reviewers: Vikas Singh <vikas@confluent.io>, Jason Gustafson <jason@confluent.io>, Shailesh Panwar <spanwar@confluent.io> (cherry picked from commit 7e36865)
No description provided.