You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "codenohup (via GitHub)" <gi...@apache.org> on 2025/02/17 17:20:25 UTC
[PR] [CELEBORN-1867][Flink] Fix flink client memory leak due to incorrect handing addCredit and notifyRequiredSegment RPCs [celeborn]
codenohup opened a new pull request, #3103:
URL: https://github.com/apache/celeborn/pull/3103
### What changes were proposed in this pull request?
When I tested the performance of Flink with Celeborn in session mode, using both the shuffle-service plugin and hybrid shuffle integration strategies, I noticed that the task heap continuously grew even when no jobs were running.
The issue arises because the Celeborn client sends addCredit or notifyRequiredSegment requests, expecting a response. This creates a callback and maintains a reference to CelebornBufferStream, SingleInputGate, and StreamTask.
These callbacks are stored in TransportResponseHandler#outstandingRpcs and are cleared upon receiving a response.
However, the Worker does not send a response for these two RPC requests, which leads to a significant memory leak in the client.
In my opinion, these two RPCs only need to be sent and checked for successful delivery; there’s no need for the Worker to send a response. We could add a new method, TransportClient#sendRpcWithCheckSuccess. This method would also add a listener to TransportResponseHandler#outstandingRpcs and Netty#writeAndFlush, but it would be cleared upon successful sending rather than waiting for a response.
### Why are the changes needed?
It may cause flink client memory leak.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Test by rerun TPCDS in Flink session mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1867][Flink] Fix flink client memory leak due to incorrect handing addCredit and notifyRequiredSegment RPCs [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3103:
URL: https://github.com/apache/celeborn/pull/3103#issuecomment-2663747562
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3103?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `84.61538%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 32.77%. Comparing base [(`fdf1883`)](https://app.codecov.io/gh/apache/celeborn/commit/fdf1883f2520f4e7a18f7bd934dece7f2cb5b2b3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ee6c78b`)](https://app.codecov.io/gh/apache/celeborn/commit/ee6c78b6714641cc25ad185a312a329eca1cc8f0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 14 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/celeborn/pull/3103?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...eleborn/common/network/client/TransportClient.java](https://app.codecov.io/gh/apache/celeborn/pull/3103?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Fclient%2FTransportClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9jbGllbnQvVHJhbnNwb3J0Q2xpZW50LmphdmE=) | 88.89% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/3103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ommon/network/client/TransportResponseHandler.java](https://app.codecov.io/gh/apache/celeborn/pull/3103?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Fclient%2FTransportResponseHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9jbGllbnQvVHJhbnNwb3J0UmVzcG9uc2VIYW5kbGVyLmphdmE=) | 75.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/3103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3103 +/- ##
==========================================
+ Coverage 32.62% 32.77% +0.15%
==========================================
Files 336 337 +1
Lines 20083 20120 +37
Branches 1798 1802 +4
==========================================
+ Hits 6551 6592 +41
+ Misses 13165 13162 -3
+ Partials 367 366 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/3103?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1867][Flink] Fix flink client memory leak of TransportResponseHandler#outstandingRpcs for handling addCredit and notifyRequiredSegment response [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #3103: [CELEBORN-1867][Flink] Fix flink client memory leak of TransportResponseHandler#outstandingRpcs for handling addCredit and notifyRequiredSegment response
URL: https://github.com/apache/celeborn/pull/3103
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1867][Flink] Fix flink client memory leak of TransportResponseHandler#outstandingRpcs for handling addCredit and notifyRequiredSegment response [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3103:
URL: https://github.com/apache/celeborn/pull/3103#issuecomment-2673532395
Thanks, Merge to main(v0.6.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1867][Flink] Fix flink client memory leak due to incorrect handing addCredit and notifyRequiredSegment RPCs [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3103:
URL: https://github.com/apache/celeborn/pull/3103#issuecomment-2664538778
OperationComplete does not guarantee that the RPC was received and processed correctly by the server. For RpcRequest methods such as addCredit and notifyRequiredSegment, the client needs to ensure and verify Worker correctly ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1867][Flink] Fix flink client memory leak of TransportResponseHandler#outstandingRpcs for handling addCredit and notifyRequiredSegment response [celeborn]
Posted by "codenohup (via GitHub)" <gi...@apache.org>.
codenohup commented on PR #3103:
URL: https://github.com/apache/celeborn/pull/3103#issuecomment-2667387777
Hi, @RexXiong
You are correct. I have modified this PR according to your suggestions. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org