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

do not initialize message slice in writer with max capacity #1193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zachxu42
Copy link

@zachxu42 zachxu42 commented Sep 8, 2023

The benefit of initializing the slice with max batch size as the capacity is minimum compared to the downside - in applications that uses large max batch size but writes frequent small batches. We saw memory allocation dropped from 30GiB per minute to 100MB after this change.

@petedannemann
Copy link
Contributor

I don't think this is a change we can accommodate due to how much it could disrupt users of this library. If you application is doing frequent small writes why not just decrease your Writer.BatchBytes?

@petedannemann petedannemann self-assigned this Sep 22, 2023
@zachxu42
Copy link
Author

zachxu42 commented Sep 22, 2023

Thanks @petedannemann for the comment. We have frequent small writes but also occasional large writes. Ideally the batch size and batch bytes can be adjusted dynamically based on the write pressure. Using a large batch size is our work around to avoid many batches get created when there's a burst of traffic, which causes a whole other set of issues (e.g. a burst of write dials could cause brief but noticeable performance issue in the broker). A lot of these are trade offs, and ideally the library should allow the user to make these trade offs.

I wonder what's the recommendation for users of this library that require lower level tuning like this..? Putting things behind configs/flags, or forking the library are the only 2 things I can think of. Neither is maintainable in the long run 😮‍💨

@petedannemann
Copy link
Contributor

I agree with the sentiment, I think making things more configurable to support lower level use cases like this is really the only option as you mentioned. It will come with a maintenance cost but if it is desired I don't really see another way forward

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.

None yet

2 participants