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

netty: Removed 4096 min buffer size #11856

Merged
merged 2 commits into from
Jan 30, 2025
Merged

Conversation

AgraVator
Copy link
Contributor

@AgraVator AgraVator commented Jan 27, 2025

Fixes #11719
MessageFramer.flush() is being called between every message, so messages are never combined and the larger allocation just wastes memory. The change removes the restriction on the min buffer size and hence, prevents the said wastage.

Copy link

linux-foundation-easycla bot commented Jan 27, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

We need to include more context. At the very least this (the commit and the PR description) should have a "Fixes #11719" to associate it with the issue (and it will also auto-close the issue when merging). But we should also summarize why we are making the change. I'd say something like, "MessageFramer.flush() is being called between every message, so messages are never combined and the larger allocation just wastes memory."

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

You'll want to "squash and merge" using the github button. It will give you the opportunity to write the commit message, and you'll want to clean it up, because it gives a very poor default (it is a useful default, as it gives you all the words you may want to adjust, but it will need editing).

@ejona86
Copy link
Member

ejona86 commented Jan 29, 2025

It will give you the opportunity to write the commit message, and you'll want to clean it up

Yeah, looks like you'll want to copy from your PR description to fill in the commit description.

@AgraVator AgraVator merged commit 7153ff8 into master Jan 30, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants