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][client] Reserve allocated buffer in BatchMessageContainer on client memory limitation. #17936

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Oct 5, 2022

Motivation

We should reserve the allocated buffer in BatchMessageContainer on client memory limitation.

Modifications

In this patch, we only take care of the buffer init size and final size

  • Added a variable batchAllocatedSize in BatchMessageContainer to record the current allocated buffer size. It is used to release the allocated buffer size from memory limit controller

  • Added a method reserveBatchAllocatedSizeWhenInit() in BatchMessageContainer, will be call when init the allocated buffer. This method will initiliaze the batchAllocatedSize and reserve memory to the memory limit controller.

  • Added a method updateBatchAllocatedSizeWhenLeave() in BatchMessageContainer, will be call when the batch leaving the BatchMessageContainer. This method will update the final batchAllocatedSize as the size after compress.

  • Then the value of batchAllocatedSize will be added to OpSendMsg#uncompressedSize for release the batchAllocatedSize memory from memory limit controller when send succesfully or failed.

  • The following diagram could be helpful to understand this patch

image

Verifying this change

  • Make sure that the change passes the CI checks.
  • The following tests have covered this change:
    • org.apache.pulsar.client.api.MemoryLimitTest
    • org.apache.pulsar.client.impl.ProducerMemoryLimitTest

Documentation

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

Matching PR in forked repository

PR in forked repository: AnonHxy#9

@AnonHxy AnonHxy self-assigned this Oct 5, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 5, 2022
@AnonHxy AnonHxy force-pushed the correct_batch_mem branch from 284e110 to c5e3d16 Compare October 5, 2022 04:36
@AnonHxy AnonHxy added the type/bug The PR fixed a bug or issue reported a bug label Oct 5, 2022
@AnonHxy AnonHxy force-pushed the correct_batch_mem branch from a37be35 to 32ef485 Compare October 9, 2022 09:18
@AnonHxy AnonHxy changed the title [fix][java-client]Take into account the buffer allocated by BatchMessageContainerImpl when limiting then memory used [fix][java-client]Take into account the buffer allocated by BatchMessageContainer when limiting then memory used Oct 9, 2022
@AnonHxy AnonHxy changed the title [fix][java-client]Take into account the buffer allocated by BatchMessageContainer when limiting then memory used [fix][java-client]Take into account the buffer allocated by BatchMessageContainer when limiting the memory used Oct 9, 2022
@AnonHxy AnonHxy added this to the 2.12.0 milestone Oct 9, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Oct 9, 2022

@AnonHxy AnonHxy changed the title [fix][java-client]Take into account the buffer allocated by BatchMessageContainer when limiting the memory used [fix][client]Take into account the buffer allocated by BatchMessageContainer when limiting the memory used Oct 11, 2022
@HQebupt HQebupt self-requested a review October 18, 2022 02:33
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Oct 18, 2022

@codelipenghui @Jason918 @merlimat PTAL :)

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Oct 18, 2022

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@2b5b92c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17936   +/-   ##
=========================================
  Coverage          ?   49.39%           
  Complexity        ?     6908           
=========================================
  Files             ?      393           
  Lines             ?    43424           
  Branches          ?     4464           
=========================================
  Hits              ?    21451           
  Misses            ?    19595           
  Partials          ?     2378           
Flag Coverage Δ
unittests 49.39% <0.00%> (?)

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

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor comment.

@Jason918 Jason918 changed the title [fix][client]Take into account the buffer allocated by BatchMessageContainer when limiting the memory used [fix][client] Reserve allocated buffer in BatchMessageContainer on client memory limitation. Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants