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

[ISSUE-553] Avoid removing buffer multiple times when clearing resources #534

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

xianjingfeng
Copy link
Member

What changes were proposed in this pull request?

Avoid remove buffer multiple times when clear resource

Why are the changes needed?

This will cause memory to be released multiple times, Fix #533

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need

@xianjingfeng xianjingfeng requested a review from zuston February 1, 2023 02:25
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Merging #534 (e75bce9) into master (ebbe2db) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #534      +/-   ##
============================================
- Coverage     60.21%   60.21%   -0.01%     
  Complexity     1784     1784              
============================================
  Files           205      205              
  Lines         11557    11556       -1     
  Branches       1042     1042              
============================================
- Hits           6959     6958       -1     
  Misses         4190     4190              
  Partials        408      408              
Impacted Files Coverage Δ
...he/uniffle/server/buffer/ShuffleBufferManager.java 83.27% <100.00%> (-0.06%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@advancedxy
Copy link
Contributor

Seems there are multiple instances for this double release.
Is it possible to add a ut to simulate the long app clean situation and make sure we don't encounter this problem later.

if is too much trouble, we than defer that in later improvement.

zuston
zuston previously approved these changes Feb 1, 2023
@zuston
Copy link
Member

zuston commented Feb 1, 2023

LGTM.

Seems there are multiple instances for this double release.
Is it possible to add a ut to simulate the long app clean situation and make sure we don't encounter this problem later.
if is too much trouble, we than defer that in later improvement.

+1. I will merge this if it still has too much work to do when adding tests.

@xianjingfeng
Copy link
Member Author

Seems there are multiple instances for this double release. Is it possible to add a ut to simulate the long app clean situation and make sure we don't encounter this problem later.

if is too much trouble, we than defer that in later improvement.

I will try to add this ut in this pr.

@xianjingfeng
Copy link
Member Author

Similar ut already exist before.

public void clearMultiTimesTest() throws Exception {

And a new ut is added for this pr. PTAL. @zuston @advancedxy

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM

@zuston zuston changed the title [ISSUE-553] Avoid remove buffer multiple times when clear resource [ISSUE-553] Avoid removing buffer multiple times when clearing resources Feb 2, 2023
@zuston zuston merged commit bc474e8 into apache:master Feb 2, 2023
@zuston
Copy link
Member

zuston commented Feb 2, 2023

Thanks @xianjingfeng @advancedxy. Merged.

@xianjingfeng xianjingfeng deleted the issue_533 branch August 9, 2024 06:45
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.

[Improvement] Avoid remove buffer multiple times when clear resource for expired application
4 participants