Skip to content
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

[improve] Improve logic for enabling Netty leak detection #23613

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 20, 2024

Motivation

Currently it's tricky and confusing to enable the Netty leak detector for Pulsar broker and Pulsar clients.

The current logic expects that pulsar.allocator.leak_detection is set. The standard property for Netty is io.netty.leakDetection.level (legacy property io.netty.leakDetectionLevel).

One additional challenge in setting the pulsar.allocator.leak_detection property is that the value is case sensitive. It must be set to Disabled, Simple, Advanced or Paranoid instead of disabled, simple, advanced or paranoid.

Modifications

  • Allow setting the Netty leak detector for Pulsar brokers and Pulsar clients by setting any one of the following system properties:
    • io.netty.leakDetectionLevel (legacy Netty property)
    • io.netty.leakDetection.level (standard Netty property)
    • pulsar.allocator.leak_detection (Pulsar's property)
  • When multiple properties are set, use the setting with the highest leak detection level.
  • Use case insensitive parsing of the setting since it's currently case sensitive.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 4.1.0 milestone Nov 20, 2024
@lhotari lhotari self-assigned this Nov 20, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 20, 2024
@lhotari lhotari added ready-to-test and removed doc-not-needed Your PR changes do not impact docs labels Nov 20, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 20, 2024
@lhotari lhotari changed the title [improve][common] Improve logic for enabling Netty leak detection [improve] Improve logic for enabling Netty leak detection Nov 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.37%. Comparing base (bbc6224) to head (6d7cc65).
Report is 743 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23613      +/-   ##
============================================
+ Coverage     73.57%   74.37%   +0.80%     
- Complexity    32624    34987    +2363     
============================================
  Files          1877     1944      +67     
  Lines        139502   147134    +7632     
  Branches      15299    16224     +925     
============================================
+ Hits         102638   109438    +6800     
- Misses        28908    29253     +345     
- Partials       7956     8443     +487     
Flag Coverage Δ
inttests 27.52% <100.00%> (+2.93%) ⬆️
systests 24.36% <100.00%> (+0.03%) ⬆️
unittests 73.76% <100.00%> (+0.91%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ulsar/common/allocator/PulsarByteBufAllocator.java 68.29% <100.00%> (+5.43%) ⬆️

... and 660 files with indirect coverage changes

---- 🚨 Try these New Features:

@lhotari lhotari merged commit 949750f into apache:master Nov 21, 2024
52 checks passed
lhotari added a commit that referenced this pull request Nov 21, 2024
lhotari added a commit that referenced this pull request Nov 21, 2024
lhotari added a commit that referenced this pull request Nov 21, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants