KAFKA-7660: Fix child sensor memory leak#5974
KAFKA-7660: Fix child sensor memory leak#5974guozhangwang merged 2 commits intoapache:trunkfrom vvcephei:fix-child-sensor-memory-leak
Conversation
| return this.name; | ||
| } | ||
|
|
||
| List<Sensor> parents() { |
There was a problem hiding this comment.
Can we add package-private methods without a KIP?
|
@guozhangwang do you mind taking a look at this? |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
LGTM! Waiting for unit test to pass.
| return this.name; | ||
| } | ||
|
|
||
| List<Sensor> parents() { |
| log.debug("Removed sensor with name {}", name); | ||
| childSensors = childrenSensors.remove(sensor); | ||
| for (final Sensor parent : sensor.parents()) { | ||
| childrenSensors.getOrDefault(parent, emptyList()).remove(sensor); |
There was a problem hiding this comment.
It's really weird that we store a reference to the parent sensors inside the Sensor object, but children sensors are kept in a separate map. 😕
There was a problem hiding this comment.
I frowned upon it as well, and for the history the parent stored on sensor was there since day one, while the children map is added a year later. I think it's actually better to keep both the parents and children inside a single Sensor, but we can do that in another cleanup PR.
There was a problem hiding this comment.
Yeah, I fought the urge to refactor it into a more traditional bidirectionally linked graph structure.
Maybe later; it doesn't seem worth opening that can of worms right now.
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks, left a couple minor comments.
|
|
||
| final Sensor parent = metrics.sensor("parent"); | ||
| metrics.sensor("child", parent); | ||
|
|
There was a problem hiding this comment.
Maybe worth a sanity assertion before removal of the child sensor? Something like this:
assertTrue(metrics.childrenSensors().get(parent).contains(child));| removeMetric(metric.metricName()); | ||
| log.debug("Removed sensor with name {}", name); | ||
| childSensors = childrenSensors.remove(sensor); | ||
| for (final Sensor parent : sensor.parents()) { |
There was a problem hiding this comment.
Hmm.. On sensor creation, we have a check if parents is null. Should we have that check here as well or is it unneeded in the creation logic?
There was a problem hiding this comment.
Not sure I follow: since we check and avoid nulls at the creation time already, parents() call is hence always safe to return not-null, right?
There was a problem hiding this comment.
I'm ashamed I didn't think of this. I think it's not needed here, since the sensor's constructor ensures that the parents field is not null. If the constructor arg is null, the field is initialized as an empty array.
Does that seem legit?
There was a problem hiding this comment.
Ah yeah, we do have a null check in the constructor. Good enough for me.
|
Ok @hachikuji , I spruced up the test a bit. |
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>
4 participants
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.
Committer Checklist (excluded from commit message)