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

[fix][offload] Fix memory leak while Offloading ledgers #18500

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

eolivelli
Copy link
Contributor

Motivation

  • BlockAwareSegmentInputStreamImpl never releases the BookKeeper entries in the close method
  • This leads to OutOfDirectMemory errors on brokers that run frequently Offloading activity

Modifications

Fix the "if" condition.

Verifying this change

This change added tests

Documentation

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

- BlockAwareSegmentInputStreamImpl never releases the BookKeeper entries in the close method
@eolivelli eolivelli self-assigned this Nov 16, 2022
@eolivelli eolivelli requested review from zymap and hangc0276 November 16, 2022 10:58
@eolivelli eolivelli added this to the 2.12.0 milestone Nov 16, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 16, 2022
@eolivelli eolivelli added release/2.11.0 ready-to-test and removed doc-not-needed Your PR changes do not impact docs labels Nov 16, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 16, 2022
@eolivelli eolivelli requested a review from jiazhai November 16, 2022 11:03
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #18500 (a16e88c) into master (7975023) will increase coverage by 2.04%.
The diff coverage is 59.37%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18500      +/-   ##
============================================
+ Coverage     45.62%   47.66%   +2.04%     
+ Complexity    10075     9337     -738     
============================================
  Files           697      617      -80     
  Lines         68024    58527    -9497     
  Branches       7293     6105    -1188     
============================================
- Hits          31033    27898    -3135     
+ Misses        33413    27606    -5807     
+ Partials       3578     3023     -555     
Flag Coverage Δ
unittests 47.66% <59.37%> (+2.04%) ⬆️

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

Impacted Files Coverage Δ
...nt/impl/PulsarClientImplementationBindingImpl.java 72.41% <ø> (-0.47%) ⬇️
...ar/client/impl/conf/ConsumerConfigurationData.java 92.55% <ø> (+2.12%) ⬆️
...he/pulsar/client/impl/TypedMessageBuilderImpl.java 30.76% <59.37%> (+3.56%) ⬆️
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...apache/pulsar/client/impl/AutoClusterFailover.java 70.00% <0.00%> (-6.12%) ⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 55.83% <0.00%> (-3.46%) ⬇️
...pache/pulsar/broker/admin/v2/PersistentTopics.java 71.68% <0.00%> (-2.14%) ⬇️
...rg/apache/pulsar/broker/web/PulsarWebResource.java 56.43% <0.00%> (-1.87%) ⬇️
...pache/pulsar/broker/admin/impl/NamespacesBase.java 62.05% <0.00%> (-1.78%) ⬇️
... and 149 more

@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/tieredstorage and removed release/2.9.0 labels Nov 16, 2022
@codelipenghui codelipenghui changed the title [fix] Fix memory leak while Offloading ledgers [fix][offload] Fix memory leak while Offloading ledgers Nov 16, 2022
@eolivelli eolivelli merged commit 6ff7d45 into apache:master Nov 16, 2022
@eolivelli eolivelli deleted the impl/leak-offloaders branch November 16, 2022 12:44
eolivelli added a commit that referenced this pull request Nov 16, 2022
eolivelli added a commit that referenced this pull request Nov 16, 2022
eolivelli added a commit that referenced this pull request Nov 16, 2022
eolivelli added a commit to datastax/pulsar that referenced this pull request Nov 16, 2022
@hangc0276
Copy link
Contributor

Nice catch!

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Good work @eolivelli

@lhotari
Copy link
Member

lhotari commented Nov 16, 2022

Tagging #15063 since that's where the issue slipped in.

congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
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.

6 participants