KAFKA-7660: fix parentSensors memory leak#5953
KAFKA-7660: fix parentSensors memory leak#5953guozhangwang merged 4 commits intoapache:trunkfrom vvcephei:parentsensors-memory-leak
Conversation
|
@guozhangwang @bbejeck do you mind taking a look at this if you have the chance? |
| } | ||
| } | ||
|
|
||
| /** visible for testing */ |
There was a problem hiding this comment.
We usually avoid the get prefix in cases like this
| metrics.removeSensor(sensor.name()); | ||
|
|
||
| final Sensor parent = parentSensors.get(sensor); | ||
| final Sensor parent = parentSensors.remove(sensor); |
|
failures unrelated retest this please |
|
@vvcephei Build failed with checkstyle error. |
|
Hi all, I've addressed the comments and fixed the error. I found an additional leak in I took the opportunity to refactor the other ( I don't plan to tack on anything else to this PR. Do you mind taking another look? Thanks, |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
@vvcephei Thanks for the updated patch! I just had a minor question on unit test and lgtm otherwise.
|
|
||
| assertThat(registry.metrics().size(), equalTo(numberOfTaskMetrics)); | ||
|
|
||
| final Sensor parent2 = metrics.taskLevelSensor(taskName, operation, Sensor.RecordingLevel.DEBUG); |
There was a problem hiding this comment.
What is this used for? Sensor parent1 and parent2 should be referring to exactly the same object right?
There was a problem hiding this comment.
They should be, but in reality the same parent sensor is (re)registered each time a child sensor is created, so I wanted to (idempotently) create a second parent sensor to make sure there's no extra references.
|
The test failures are unrelated: Retest this, please. |
| } | ||
|
|
||
| @Test | ||
| public void testMutiLevelSensorRemoval() { |
There was a problem hiding this comment.
Typo: testMu[l]tiLevelSensorRemoval() {
In StreamsMetricsImpl, the parentSensors map was keeping references to Sensors after the sensors themselves had been removed. Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
6 participants
In StreamsMetricsImpl, the parentSensors map was keeping references to Sensors after the sensors themselves had been removed.
Committer Checklist (excluded from commit message)