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
[FLINK-22050][network] Recycle all buffers before failing releaseAllBuffers #15440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this fix. I think we should always try to do best effort resource freeing even with intermittent errors.
Since, we are not super performance-critical here, I'd suggest to use Guava's Closer instead since that handles suppression transparently.
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 8641227 (Sat Aug 28 11:11:42 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
3a6408f
to
43120be
Compare
Thanks for quick review. |
43120be
to
6bfa1fc
Compare
I am always sceptical about using third party libraries, especially shaded ones if we have our own alternatives. Could we use |
One reason why |
In the PR description it’s missing if it’s fixing the problem from the Jira ticket or not. secondly please copy paste this description to the commit message as well. |
6bfa1fc
to
2f7c0b2
Compare
Done |
2f7c0b2
to
219336a
Compare
I had to revert back to try blocks (instead of Closer) as LIFO order is apparently important here. |
Currently, BufferManager.releaseAllBuffers fails as soon as the recycling of the first buffer fails. This is correct as it likely caused by programmer error. However, this might cause resource leak and lead to subsequent test failures. After this change, it first tries to recycle all buffers and then throws exception if any.
219336a
to
8641227
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's merge it when AZP agrees.
Thanks for reviewing! |
What is the purpose of the change
Currently,
BufferManager.releaseAllBuffers
fails as soon as the recycling of the first buffer fails.This is correct as it likely caused by programmer error.
However, this might cause resource leak and lead to subsequent test failures.
With this PR, it first tries to recycle all buffers and then throws exception if any.
(this is an independent improvement; not solving the original problem)
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation