Skip to content

Conversation

@summaryzb
Copy link
Contributor

What changes were proposed in this pull request?

the behavior of buffer management and memory allocate
when adding a huge record to a partition where the buffer is not initialized, we only allocate bufferSegmentSize memory, however release the real huge size memory after the buffer is sent.
this will easily cause OOM.

Why are the changes needed?

better manage memory

Does this PR introduce any user-facing change?

no

How was this patch tested?

pass testing

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2022

Codecov Report

Merging #145 (5b9ea91) into master (04cbdbb) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #145      +/-   ##
============================================
+ Coverage     57.20%   57.22%   +0.02%     
- Complexity     1200     1201       +1     
============================================
  Files           150      150              
  Lines          8185     8183       -2     
  Branches        773      772       -1     
============================================
+ Hits           4682     4683       +1     
+ Misses         3257     3255       -2     
+ Partials        246      245       -1     
Impacted Files Coverage Δ
...pache/spark/shuffle/writer/WriteBufferManager.java 83.45% <100.00%> (+1.23%) ⬆️
...e/uniffle/server/storage/SingleStorageManager.java 67.21% <0.00%> (+1.63%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jerqi jerqi changed the title [Bugfix]Fix memory leak which cause oom [BUGFIX] Fix memory leak which cause oom Aug 9, 2022
@jerqi
Copy link
Contributor

jerqi commented Aug 9, 2022

LGTM, cc @colinmjj

Copy link

@colinmjj colinmjj left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@jerqi jerqi merged commit b3c0752 into apache:master Aug 10, 2022
@jerqi
Copy link
Contributor

jerqi commented Aug 10, 2022

Thanks @summaryzb, Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants