Skip to content

KAFKA-7660: Fix child sensor memory leak #5974

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.

Sign up for GitHub

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

Merged
guozhangwang merged 2 commits into from
Nov 30, 2018
Merged

KAFKA-7660: Fix child sensor memory leak #5974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1398f42
KAFKA-7660: Fix child sensor memory leak
vvcephei Nov 29, 2018
5f6aea2
cr comment: test improvement
vvcephei Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;

import static java.util.Collections.emptyList;

/**
* A registry of sensors and metrics.
* <p>
Expand Down Expand Up @@ -446,6 +448,9 @@ public void removeSensor(String name) {
removeMetric(metric.metricName());
log.debug("Removed sensor with name {}", name);
childSensors = childrenSensors.remove(sensor);
for (final Sensor parent : sensor.parents()) {

@hachikuji hachikuji Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@guozhangwang guozhangwang Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@vvcephei vvcephei Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@hachikuji hachikuji Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, we do have a null check in the constructor. Good enough for me.

childrenSensors.getOrDefault(parent, emptyList()).remove(sensor);

@hachikuji hachikuji Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😕

@guozhangwang guozhangwang Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vvcephei vvcephei Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.kafka.common.utils.Utils;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedList;
Expand All @@ -32,6 +31,9 @@
import java.util.Set;
import java.util.concurrent.TimeUnit;

import static java.util.Arrays.asList;
import static java.util.Collections.unmodifiableList;

/**
* A sensor applies a continuous sequence of numerical values to a set of associated metrics. For example a sensor on
* message size would record a sequence of message sizes using the {@link #record(double)} api and would maintain a set
Expand Down Expand Up @@ -133,6 +135,10 @@ public String name() {
return this.name;
}

List<Sensor> parents() {

@vvcephei vvcephei Nov 29, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add package-private methods without a KIP?

@guozhangwang guozhangwang Nov 29, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine.

return unmodifiableList(asList(parents));
}

/**
* Record an occurrence, this is just short-hand for {@link #record(double) record(1.0)}
*/
Expand Down Expand Up @@ -291,7 +297,7 @@ public boolean hasExpired() {
}

synchronized List<KafkaMetric> metrics() {
return Collections.unmodifiableList(new LinkedList<>(this.metrics.values()));
return unmodifiableList(new LinkedList<>(this.metrics.values()));
}

/**
Expand Down
16 changes: 16 additions & 0 deletions clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package org.apache.kafka.common.metrics;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -202,6 +204,20 @@ public void testBadSensorHierarchy() {
metrics.sensor("gc", c1, c2); // should fail
}

@Test
public void testRemoveChildSensor() {
final Metrics metrics = new Metrics();

final Sensor parent = metrics.sensor("parent");
final Sensor child = metrics.sensor("child", parent);

assertEquals(singletonList(child), metrics.childrenSensors().get(parent));

@hachikuji hachikuji Nov 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth a sanity assertion before removal of the child sensor? Something like this:

assertTrue(metrics.childrenSensors().get(parent).contains(child));

@vvcephei vvcephei Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.

metrics.removeSensor("child");

assertEquals(emptyList(), metrics.childrenSensors().get(parent));
}

@Test
public void testRemoveSensor() {
int size = metrics.metrics().size();
Expand Down