Skip to content

Pick the higher leak detection level between netty and bookkeeper. #3794

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
hangc0276 merged 8 commits into from
Mar 15, 2023
Merged

Pick the higher leak detection level between netty and bookkeeper. #3794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9ad6fc
Pick the higher leak detection level between netty and bookkeeper.
horizonzy Feb 21, 2023
0e8862a
Fix style.
horizonzy Feb 21, 2023
ffabe49
Fix style.
horizonzy Feb 21, 2023
d969540
Fix check style.
horizonzy Feb 21, 2023
f828eab
Fix check style.
horizonzy Feb 22, 2023
e714706
Hint the log info.
horizonzy Feb 22, 2023
49c29b3
Merge remote-tracking branch 'upstream/master' into pick-higher-level…
horizonzy Mar 14, 2023
47f181d
Address the comments.
horizonzy Mar 14, 2023
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
17 changes: 16 additions & 1 deletion ...n-allocator/src/main/java/org/apache/bookkeeper/common/allocator/LeakDetectionPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@
*/
package org.apache.bookkeeper.common.allocator;

import lombok.extern.slf4j.Slf4j;

/**
* Define the policy for the Netty leak detector.
*/
@Slf4j
public enum LeakDetectionPolicy {

/**
Expand All @@ -43,5 +46,17 @@ public enum LeakDetectionPolicy {
* stack traces of places where the buffer was used. Introduce very
* significant overhead.
*/
Paranoid,
Paranoid;

public static LeakDetectionPolicy parseLevel(String levelStr) {
String trimmedLevelStr = levelStr.trim();
for (LeakDetectionPolicy policy : values()) {
if (trimmedLevelStr.equalsIgnoreCase(policy.name())) {
return policy;
}
}
log.warn("Parse leak detection policy level {} failed. Use the default level: {}", levelStr,
LeakDetectionPolicy.Disabled.name());
return LeakDetectionPolicy.Disabled;

@eolivelli eolivelli Feb 21, 2023

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.

we should throw an error, otherwise it is possible that you configure it and you set a wrong value.

if you don't want to throw then we should at least log something at ERROR level

@horizonzy horizonzy Feb 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Add warning log when parsing level string failed.

@hangc0276 hangc0276 Feb 22, 2023

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.

We'd better print out the levelStr in the log. Maybe we need to change the log level to ERROR instead of WARN?

@horizonzy horizonzy Feb 22, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We'd better print out the levelStr in the log

Make sense

Maybe we need to change the log level to ERROR instead of WARN?

I think warn is enough. It can work well. If we throw an exception, we can use ERROR log.

@hangc0276 hangc0276 Mar 13, 2023

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.

Use: log.warn("Parse leak detection policy level {} failed. Use the default level: {}", levelStr, LeakDetectionPolicy.Disabled.name());

}
}
13 changes: 11 additions & 2 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,8 +1129,17 @@ public T setAllocatorOutOfMemoryPolicy(OutOfMemoryPolicy oomPolicy) {
* Return the configured leak detection policy for the allocator.
*/
public LeakDetectionPolicy getAllocatorLeakDetectionPolicy() {
return LeakDetectionPolicy
.valueOf(this.getString(ALLOCATOR_LEAK_DETECTION_POLICY, LeakDetectionPolicy.Disabled.toString()));
//see: https://lists.apache.org/thread/d3zw8bxhlg0wxfhocyjglq0nbxrww3sg
String nettyLevelStr = System.getProperty("io.netty.leakDetectionLevel", LeakDetectionPolicy.Disabled.name());
nettyLevelStr = System.getProperty("io.netty.leakDetection.level", nettyLevelStr);
String bkLevelStr = getString(ALLOCATOR_LEAK_DETECTION_POLICY, LeakDetectionPolicy.Disabled.name());
LeakDetectionPolicy nettyLevel = LeakDetectionPolicy.parseLevel(nettyLevelStr);
LeakDetectionPolicy bkLevel = LeakDetectionPolicy.parseLevel(bkLevelStr);
if (nettyLevel.ordinal() >= bkLevel.ordinal()) {
return nettyLevel;
} else {
return bkLevel;
}
}

/**
Expand Down
49 changes: 49 additions & 0 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/conf/AbstractConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.mock;

import org.apache.bookkeeper.common.allocator.LeakDetectionPolicy;
import org.apache.bookkeeper.meta.AbstractZkLedgerManagerFactory;
import org.apache.bookkeeper.meta.HierarchicalLedgerManagerFactory;
import org.apache.bookkeeper.meta.LedgerManagerFactory;
Expand Down Expand Up @@ -130,4 +131,52 @@ public void testUnknownZkLedgerManagerFactory() throws Exception {
conf.getMetadataServiceUri();
}

@Test
public void testAllocatorLeakDetectionPolicy() {
String nettyOldLevelKey = "io.netty.leakDetectionLevel";
String nettyLevelKey = "io.netty.leakDetection.level";

String nettyOldLevelStr = System.getProperty(nettyOldLevelKey);
String nettyLevelStr = System.getProperty(nettyLevelKey);

//Remove netty property for test.
System.getProperties().remove(nettyOldLevelKey);
System.getProperties().remove(nettyLevelKey);

assertEquals(LeakDetectionPolicy.Disabled, conf.getAllocatorLeakDetectionPolicy());

System.getProperties().put(nettyOldLevelKey, "zazaza");
assertEquals(LeakDetectionPolicy.Disabled, conf.getAllocatorLeakDetectionPolicy());

conf.setProperty(AbstractConfiguration.ALLOCATOR_LEAK_DETECTION_POLICY, "zazaza");
assertEquals(LeakDetectionPolicy.Disabled, conf.getAllocatorLeakDetectionPolicy());

System.getProperties().put(nettyOldLevelKey, "simple");
assertEquals(LeakDetectionPolicy.Simple, conf.getAllocatorLeakDetectionPolicy());

System.getProperties().put(nettyLevelKey, "disabled");
assertEquals(LeakDetectionPolicy.Disabled, conf.getAllocatorLeakDetectionPolicy());

System.getProperties().put(nettyLevelKey, "advanCed");
assertEquals(LeakDetectionPolicy.Advanced, conf.getAllocatorLeakDetectionPolicy());

conf.setProperty(AbstractConfiguration.ALLOCATOR_LEAK_DETECTION_POLICY, "simPle");
assertEquals(LeakDetectionPolicy.Advanced, conf.getAllocatorLeakDetectionPolicy());

conf.setProperty(AbstractConfiguration.ALLOCATOR_LEAK_DETECTION_POLICY, "advanCed");
assertEquals(LeakDetectionPolicy.Advanced, conf.getAllocatorLeakDetectionPolicy());

conf.setProperty(AbstractConfiguration.ALLOCATOR_LEAK_DETECTION_POLICY, "paranoiD");
assertEquals(LeakDetectionPolicy.Paranoid, conf.getAllocatorLeakDetectionPolicy());

System.getProperties().remove(nettyOldLevelKey);
System.getProperties().remove(nettyLevelKey);
//Revert the netty properties.
if (nettyOldLevelStr != null) {
System.getProperties().put(nettyOldLevelKey, nettyOldLevelStr);
}
if (nettyLevelStr != null) {
System.getProperties().put(nettyLevelKey, nettyLevelStr);
}
}
}