Skip to content

[#1335] fix(server)(netty): release bytebuf explicitly when requiredId is expired and cache failed - #1357

Merged
zuston merged 2 commits into
apache:masterfrom
zuston:issue-1355
Dec 8, 2023
Merged

[#1335] fix(server)(netty): release bytebuf explicitly when requiredId is expired and cache failed#1357
zuston merged 2 commits into
apache:masterfrom
zuston:issue-1355

Conversation

@zuston

@zuston zuston commented Dec 7, 2023

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Release bytebuf explicitly when requiredId is expired and cache failure occurs

Why are the changes needed?

Fix: #1335

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Internal production tests.

@zuston
zuston requested review from leixm and roryqi December 7, 2023 09:54
@zuston zuston linked an issue Dec 7, 2023 that may be closed by this pull request
3 tasks
@codecov-commenter

codecov-commenter commented Dec 7, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (c5be58f) 53.27% compared to head (f27f977) 54.18%.

Files Patch % Lines
...niffle/server/netty/ShuffleServerNettyHandler.java 0.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1357      +/-   ##
============================================
+ Coverage     53.27%   54.18%   +0.90%     
  Complexity     2712     2712              
============================================
  Files           418      398      -20     
  Lines         23905    21552    -2353     
  Branches       2042     2043       +1     
============================================
- Hits          12736    11677    -1059     
+ Misses        10384     9161    -1223     
+ Partials        785      714      -71     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zuston

zuston commented Dec 8, 2023

Copy link
Copy Markdown
Member Author

Could you help review this? @jerqi @leixm

@leixm

leixm commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

Nice catch!

@leixm

leixm commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

+1 LGTM.

@zuston

zuston commented Dec 8, 2023

Copy link
Copy Markdown
Member Author

+1 LGTM.

Could you help using the review approval, that I could merge this ?

@zuston zuston closed this Dec 8, 2023
@zuston zuston reopened this Dec 8, 2023
@leixm

leixm commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

+1 LGTM.

Could you help using the review approval, that I could merge this ?

Sure.

@zuston
zuston merged commit 453e6d6 into apache:master Dec 8, 2023
@zuston

zuston commented Dec 8, 2023

Copy link
Copy Markdown
Member Author

Thanks @leixm for your review. Merged

@xianjingfeng

Copy link
Copy Markdown
Member

@zuston I can't cherry-pick this pr into branch-0.8. Could you create a pr for branch-0.8?

@zuston

zuston commented Dec 13, 2023

Copy link
Copy Markdown
Member Author

@zuston I can't cherry-pick this pr into branch-0.8. Could you create a pr for branch-0.8?

Ok .

zuston added a commit to zuston/incubator-uniffle that referenced this pull request Dec 13, 2023
…tebuf explicitly when requiredId is expired and cache failed apache#1357
xianjingfeng pushed a commit that referenced this pull request Dec 14, 2023
…iredId is expired and cache failed #1357 (#1371)

### What changes were proposed in this pull request?
Release bytebuf explicitly when requiredId is expired and cache failure occurs

### Why are the changes needed?
Fix: #1335

### Does this PR introduce any user-facing change?
No.

### How was this patch tested?
Internal production tests.
zuston added a commit to zuston/incubator-uniffle that referenced this pull request Jan 18, 2024
…uiredId is expired or cache failed (apache#1357)

### What changes were proposed in this pull request?

Release bytebuf explicitly when `requiredId` is expired and cache failure occurs

### Why are the changes needed?

Fix: apache#1335

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Internal production tests.
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.

[Bug] server memory leak with netty

4 participants