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

Added a 100MB maxBuffer since the default 200kb is too small #226

Merged
merged 2 commits into from
Mar 28, 2018

Conversation

olivierboucher
Copy link
Contributor

Hi guys,

I ran into this issue where stdout maxBuffer is too small on a large proto codebase. This fixes the issue, 1MB is fair but I can adjust it per your liking.

Also, was not sure if master is the good branch, let me know and I'll re-submit

@murgatroid99
Copy link
Member

What is the impact of having a maxBuffer size that is too small?

@olivierboucher
Copy link
Contributor Author

olivierboucher commented Mar 21, 2018

The process crashes. You can see a discussion about it on the node repo: nodejs/node#9829

@murgatroid99
Copy link
Member

I think that if the impact is that bad, we should probably have a wider safety margin. This script should only be run on development machines, and it only spawns one process, so I think 100MB or even 1GB would be reasonable.

@olivierboucher
Copy link
Contributor Author

@murgatroid99 friendly ping -- submitted the proper changes

@olivierboucher olivierboucher changed the title Added a 1MB maxBuffer since the default 200kb is too small Added a 10MB maxBuffer since the default 200kb is too small Mar 26, 2018
@olivierboucher olivierboucher changed the title Added a 10MB maxBuffer since the default 200kb is too small Added a 100MB maxBuffer since the default 200kb is too small Mar 26, 2018
@murgatroid99 murgatroid99 merged commit 2574498 into grpc:master Mar 28, 2018
@olivierboucher
Copy link
Contributor Author

@murgatroid99 friendly ping. This issue is still on going since grpc-tools has to be released

@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants